Closed Bug 1430745 Opened 2 years ago Closed 2 years ago

IPC: Fix unaligned accesses in DirReaderLinux

Categories

(Core :: IPC, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8942879 - Flags: review?(continuation)
Attachment #8942879 - Flags: review?(continuation) → review?(nfroyd)
Comment on attachment 8942879 [details] [diff] [review]
0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch

Review of attachment 8942879 [details] [diff] [review]:
-----------------------------------------------------------------

I can't review this without *some* explanation of what's going wrong here.  16-byte alignment seems very aggressive.
Attachment #8942879 - Flags: review?(nfroyd)
Please see comment 2.
Flags: needinfo?(r)
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8942879 [details] [diff] [review]
> 0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch
> 
> Review of attachment 8942879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't review this without *some* explanation of what's going wrong here. 
> 16-byte alignment seems very aggressive.

I found the address of 'buf_' doesn't meet the 8-byte alignment requirements when it reinterpret cast to linux_dirent. On some RISC platforms, such as MIPS/ARM, it affect performance because a unaligned exception raised and need do simulate in kernel.

I think grow up to 16-byte may be is friendly to optimizied memcpy (128-bit SSE implementation).
Flags: needinfo?(r)
Attachment #8942879 - Flags: review?(nfroyd)
OK, thanks for explaining the problem.  I guess the 8-byte alignment requirement comes from the *int64_t members of linux_dirent, then?

I think we can fix this in one of two ways:

1) Use a union { linux_dirent d; char buf[512]; } and rewrite the code to reference appropriate members of the union.
2) Use __attribute__((aligned(8))).

It's not worth over-aligning this buffer; the code here is not performance-sensitive.
Comment on attachment 8942879 [details] [diff] [review]
0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch

16-byte alignment is too aggressive.  See previous comment.
Attachment #8942879 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #5)
> OK, thanks for explaining the problem.  I guess the 8-byte alignment
> requirement comes from the *int64_t members of linux_dirent, then?
> 
> I think we can fix this in one of two ways:
> 
> 1) Use a union { linux_dirent d; char buf[512]; } and rewrite the code to
> reference appropriate members of the union.
Good idea! This way is more versatile, even if linux_dirent changes in the future. Thank you!

> 2) Use __attribute__((aligned(8))).
> 
> It's not worth over-aligning this buffer; the code here is not
> performance-sensitive.
Attachment #8942879 - Attachment is obsolete: true
Attachment #8943092 - Flags: review?(nfroyd)
Comment on attachment 8943092 [details] [diff] [review]
0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch

Review of attachment 8943092 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!  r=me with the change below.

::: ipc/chromium/src/base/dir_reader_linux.h
@@ +90,5 @@
>  
>   private:
>    const int fd_;
> +  union {
> +    linux_dirent dirent;

Nit: I would call this `dirent_` to align with the local style.
Attachment #8943092 - Flags: review?(nfroyd) → review+
Pushed by r@hev.cc:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9019fd30399
IPC: Fix unaligned accesses in DirReaderLinux. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/f9019fd30399
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.