1430745 - IPC: Fix unaligned accesses in DirReaderLinux

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Comment 1

[hev [:hev]]

Attachment: 0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Nathan Froyd [:froydnj]]

Please see comment 2.

Comment 4

[hev [:hev]]

(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).

Comment 5

[Nathan Froyd [:froydnj]]

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 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[hev [:hev]]

(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.

Comment 8

[hev [:hev]]

Attachment: 0001-Bug-1430745-IPC-Fix-unaligned-accesses-in-DirReaderL.patch

Comment 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9019fd30399
IPC: Fix unaligned accesses in DirReaderLinux. r=froydnj

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f9019fd30399