1374218 - Incorrect alignment assertion in js/src/wasm/WasmInstance.cpp, GlobalSegment::create

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Julian Seward [:jseward]]

GlobalSegment::create(uint32_t globalDataLength) has this

  TlsData* tlsData =
      reinterpret_cast<TlsData*>(js_calloc(offsetof(TlsData, globalArea) + globalDataLength));
  ..
  MOZ_RELEASE_ASSERT((uintptr_t(tlsData) % 16) == 0);

js_calloc is a wrapper around plain calloc.  On x86-linux (32 bit),
the system calloc only returns 8-aligned memory.  So a build using
--disable-jemalloc will assert here (and I have observed it doing so.)

Comment 1

[Julian Seward [:jseward]]

Clearly we need 16-alignment here really.  The "obvious" fix would be to
use js_memalign followed by memset-zero .. except .. we don't have a 
js_memalign, AFAICS.  We could perhaps create one as a wrapper around
posix_memalign, assuming that that's available on all targets and that
our mozjemalloc also provides that.  Mike, is that the right thing to do?

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1374218-1.diff

A possible fix, per comment 1.

Comment 3

[Mike Hommey [:glandium]]

Even jemalloc doesn't guarantee more than 8 bytes alignment without using memalign or equivalent.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Even jemalloc doesn't guarantee more than 8 bytes alignment without using
> memalign or equivalent.

Well, there's no guarantee, but since the allocation is larger than 8 bytes, in practice it's aligned to malloc_good_size(size), which is why we're probably never seeing this in the wild. However, that's an implementation detail of jemalloc, not a guarantee. Theoretically, we could hit this assertion on asan builds, too, although maybe the asan allocator has implementation details that make it work somehow too.

Comment 5

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #1)
> We could perhaps create one as a wrapper around
> posix_memalign, assuming that that's available on all targets and that
> our mozjemalloc also provides that.  Mike, is that the right thing to do?

Mike, what's the officially approved way to do aligned allocation in our
universe?  My patch in comment 2 uses posix_memalign as I thought that
sounded the most portable, but a try build with that fails on Windows.

Comment 6

[Mike Hommey [:glandium]]

We unfortunately don't /really/ have one. The truth is we kind of do, but only when mozjemalloc is enabled (it's posix_memalign, it's available on windows too, although there's no header defining it). (Yes, we really need to unify all that, and I'm partly doing this for other reasons, and *memalign is on my radar too)

Comment 7

[Luke Wagner [:luke]]

If sizeof(std::max_align_t) >= 16 on all systems (which we could static_assert), then if I read 3.7.4.1 and 3.11.2 correctly, the C++ std guarantees the returned memory is sufficiently aligned.  sizeof(max_align_t) = 32 on my system, but I don't know if this is true across tier 1.

Comment 8

[Mike Hommey [:glandium]]

You need to look at alignof(), not sizeof()

Comment 9

[Luke Wagner [:luke]]

Oops, right.  I had expected those to be the same, but apparently not: alignof(max_align_t) = 16.

Comment 10

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > Even jemalloc doesn't guarantee more than 8 bytes alignment without using
> > memalign or equivalent.
> 
> Well, there's no guarantee, but since the allocation is larger than 8 bytes,
> in practice it's aligned to malloc_good_size(size), which is why we're
> probably never seeing this in the wild. However, that's an implementation
> detail of jemalloc, not a guarantee. Theoretically, we could hit this
> assertion on asan builds, too, although maybe the asan allocator has
> implementation details that make it work somehow too.

ASan doesn't run on 32 bit targets, and I'd guess its 64 bit allocators
guarantee 16 alignment, which is why it never fails there.

How can we fix this in the short term?  Right now, 32 bit Linux builds done
with --disable-jemalloc (that is, configured to run on valgrind) often
segfault here because they get only 8-aligned memory.  I thought of the
usual horrible hack of allocating 8 more bytes than we actually need and
bumping the pointer along by 8 if required to make it 16-aligned.  But then
we can't free the block later unless we also store the original pointer
somewhere.

Comment 11

[Lars T Hansen [:lth]]

There are not many of these objects and they are usually fairly large anyway, so we can embed the "memory pointer" inside the TlsData object, if we want to.  The GlobalSegment destructor should be the only place where we delete a tls, so there's no particular hardship.

Comment 12

[Luke Wagner [:luke]]

Oh sorry, I missed that this was actually crashing somewhere; I thought this was theoretical.  Yeah, comment 11 sounds good.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Luke Wagner [:luke] from comment #9)
> Oops, right.  I had expected those to be the same, but apparently not:
> alignof(max_align_t) = 16.

and it's 8 on 32 bits, which is what Julian is complaining about.

It's funny how in 2017, with SIMD, this alignment problem hasn't been solved in C++... there are a bunch of things in the standard to declare field alignments, etc. but nothing to actually deal with it for heap allocations...

This whole issue makes me want to write a replace-malloc lib that purposefully returns unaligned pointers to see what breaks...

Comment 14

[Luke Wagner [:luke]]

Attachment: align-tls-data

Setting aside whether we should have our own posix_memalign (which sounds great), this patch just does that Lars suggested.

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 8880959 [details] [diff] [review]
align-tls-data

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

Short and sweet.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba901f83a5fd
Baldr: ensure alignment of TlsData (r=lth)

Comment 17

[Carsten Book [:Tomcat]]

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