1061812 - Tell memory tools about the JS heap

Status: ASSIGNED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

We've had numerous issues with uninitialized reads (mostly in Windows, since it recycles more heavily). It seems like this would be a good way to suss them out sooner.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Valgrind doesn't run on Windows. Is there another good alternative?

Comment 2

[Terrence Cole [:terrence]]

Presumably the bugs exist on other platforms as well. The reason we see them more on windows is because of it is more aggressive about not zeroing.

Comment 3

[Julian Seward [:jseward]]

(In reply to Terrence Cole [:terrence] from comment #0)
> It seems like this would be a good way to suss them out sooner.

Can you be a bit more specific about what you want to tell Valgrind?
For example, you could mark memory areas that should not be touched
for some time, so that you can catch any accesses to those areas.
But I'm just guessing here about what you want to detect.

Comment 4

[Nicholas Nethercote [inactive]]

> Valgrind doesn't run on Windows. Is there another good alternative?

One possibility is Valgrind+Wine. I don't know how robust the combination is, though. It may be more trouble than it's worth.

There's also http://www.drmemory.org/. I've never tried it.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Notify valgrind when we discard pages

Relevant comments from /usr/include/valgrind/memcheck.h:

/* Mark memory at _qzz_addr as unaddressable for _qzz_len bytes. */
#define VALGRIND_MAKE_MEM_NOACCESS(_qzz_addr,_qzz_len)           \

/* Similarly, mark memory at _qzz_addr as addressable but undefined
   for _qzz_len bytes. */
#define VALGRIND_MAKE_MEM_UNDEFINED(_qzz_addr,_qzz_len)          \

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > Valgrind doesn't run on Windows. Is there another good alternative?
> 
> One possibility is Valgrind+Wine. I don't know how robust the combination
> is, though. It may be more trouble than it's worth.

We shouldn't need to run anything on Windows at all. The bugs we're looking for should appear everywhere. They just only cause problems on Windows, probably because other platforms aggressively zero out decommitted memory. (By spec: madvise(MADV_DONTNEED) must zero mappings without a file mapped.)

(In reply to Julian Seward [:jseward] from comment #3)
> (In reply to Terrence Cole [:terrence] from comment #0)
> > It seems like this would be a good way to suss them out sooner.
> 
> Can you be a bit more specific about what you want to tell Valgrind?
> For example, you could mark memory areas that should not be touched
> for some time, so that you can catch any accesses to those areas.
> But I'm just guessing here about what you want to detect.

Terrence will probably have to give the real explanation, but as I understand it: the GC has its own allocator. The sort of bug we're looking for is where you recycle a chunk of memory that previously held fully initialized data. You write in part of an object, but incorrectly leave part of it uninitialized. On non-Windows platforms, the uninitialized memory will be zeroed out. On Windows, it may still contain the previous contents. I'm not sure exactly what path this stuff goes through, but I think this bug is probably just a matter of adding in the right valgrind annotation to js/src/gc/Memory.cpp's MarkPagesUnused.

Wait, why am I spending the time to describe this? I'll just do it.

Comment 7

[Steve Fink [:sfink] [:s:]]

That didn't go over so well: https://tbpl.mozilla.org/php/getParsedLog.php?id=47341590&tree=Try

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8483679 [details] [diff] [review]
Notify valgrind when we discard pages

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

I had thought to add the annotations at the allocator level, but I think doing it at the memory level like this will work just as well, as long as we mark memory undefined in Poison too.

I have no idea why it's going off the rails in Nursery::enable as nothing there touches gc allocated memory.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8483679 [details] [diff] [review]
Notify valgrind when we discard pages

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

This is nice, but please use the macros in mfbt/MemoryChecking.h instead so that ASAN and any other future tools will get similar notifications.

Comment 10

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> This is nice, but please use the macros in mfbt/MemoryChecking.h instead so
> that ASAN and any other future tools will get similar notifications.

Yes.  This would also avoid the ifdeffery in the patch.

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 8483679 [details] [diff] [review]
> Notify valgrind when we discard pages
> 
> Review of attachment 8483679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is nice, but please use the macros in mfbt/MemoryChecking.h instead so
> that ASAN and any other future tools will get similar notifications.

Oh, cool. I'm just a lot more familiar with valgrind than I am with mfbt. :-)

(In reply to Terrence Cole [:terrence] from comment #8)
> I had thought to add the annotations at the allocator level, but I think
> doing it at the memory level like this will work just as well, as long as we
> mark memory undefined in Poison too.

Right, this is coarser granularity. Recycled pages will start out uninitialized, and gradually become initialized as they are used. If our allocator frees up regions, valgrind won't know until the whole page is decommitted again.

But you're right! The poisoning provides another cut point to get the full deal. Though that suggests that this isn't going to find our marking iloop, since a debug build would be poisoning already?

> I have no idea why it's going off the rails in Nursery::enable as nothing
> there touches gc allocated memory.

Yeah, it's weird, especially since a local valgrind run comes up clean. Though that was just a shell run, not the browser.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8483679 [details] [diff] [review]
Notify valgrind when we discard pages

r- for failing to actually work.

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Steve Fink [:sfink] from comment #11)
> (In reply to Terrence Cole [:terrence] from comment #8)
> > I have no idea why it's going off the rails in Nursery::enable as nothing
> > there touches gc allocated memory.
> 
> Yeah, it's weird, especially since a local valgrind run comes up clean.
> Though that was just a shell run, not the browser.

But it *does* touch gc allocated memory, no? This is in initChunk:

        c.trailer.location = gc::ChunkLocationBitNursery;

Sounds like it's writing into a gc-allocated chunk to me. Which would imply that the chunk had MarkPagesUnused on it (in order for this patch to make it an invalid write), and MarkPagesInUse was not called. Which seems wrong.

Comment 14

[Terrence Cole [:terrence]]

Now that you mention it, we do treat decommitted as mapped and writable -- we never re-commit, we just start writing. This includes the chunk's initializing writes, which we're hitting here, I guess. I didn't consider it too big a deal at the time, but if it's losing us some assertion power here, we should add a recommit api.