1416666 - warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘js::detail::HashTable<...> with no trivial copy-assignment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

When I compiled m-c with gcc8, I got following warning

 0:43.90 In file included from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/TracingAPI.h:12:0,
 0:43.90                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/GCPolicyAPI.h:47,
 0:43.90                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/GCVector.h:12,
 0:43.90                  from /home/allstars/src/gcc8/js/src/jscntxt.h:15,
 0:43.90                  from /home/allstars/src/gcc8/js/src/vm/RegExpObject.h:15,
 0:43.90                  from /home/allstars/src/gcc8/js/src/builtin/RegExp.h:10,
 0:43.90                  from /home/allstars/src/gcc8/js/src/builtin/RegExp.cpp:7:
 0:43.90 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h: In instantiation of ‘void js::detail::HashTable<T, HashPolicy, AllocPolicy>::clear() [with T = js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> >; HashPolicy = js::HashMap<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::MapHashPolicy; AllocPolicy = js::SystemAllocPolicy]’:
 0:43.90 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:200:57:   required from ‘void js::HashMap<Key, Value, HashPolicy, AllocPolicy>::clear() [with Key = JSScript*; Value = mozilla::UniquePtr<char [], JS::FreePolicy>; HashPolicy = js::DefaultHasher<JSScript*>; AllocPolicy = js::SystemAllocPolicy]’
 0:43.90 /home/allstars/src/gcc8/js/src/vm/GeckoProfiler.h:201:27:   required from here
 0:43.90 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1674:19: warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘js::detail::HashTable<js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> >, js::HashMap<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry {aka class js::detail::HashTableEntry<js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> > >}’ with no trivial copy-assignment [-Wclass-memaccess]
 0:43.90              memset(table, 0, sizeof(*table) * capacity());
 0:43.90              ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:43.90 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:793:7: note: ‘js::detail::HashTable<js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> >, js::HashMap<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry {aka class js::detail::HashTableEntry<js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> > >}’ declared here
 0:43.90  class HashTableEntry
 0:43.90        ^~~~~~~~~~~~~~


The line triggers the warning is from
https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/js/public/HashTable.h#1674

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Patch

In the error message from comment 0,
js::HashMapEntry<JSScript*, mozilla::UniquePtr<char [], JS::FreePolicy> should NOT be a POD, however compiler still emits the warning.

So I seperate the clear code to a template, this patch fixes some warning, however there's still some warnings left, and I am not sure why it still has, because HashMapEntry<js::gc::Cell*, long unsigned int> should be a POD.

Jonco, can you check my patch is in the right direction?
Thanks

 3:12.75 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h: In instantiation of ‘static void js::detail::HashTableClearPolicy<<anonymous> >::clear(js::detail::HashTableEntry<T>*, uint32_t) [with T = js::HashMapEntry<js::gc::Cell*, long unsigned int>; bool isPod = true; uint32_t = unsigned int]’:
 3:12.75 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1697:27:   required from ‘void js::detail::HashTable<T, HashPolicy, AllocPolicy, ClearPolicy>::clear() [with T = js::HashMapEntry<js::gc::Cell*, long unsigned int>; HashPolicy = js::HashMap<js::gc::Cell*, long unsigned int, js::PointerHasher<js::gc::Cell*>, js::SystemAllocPolicy>::MapHashPolicy; AllocPolicy = js::SystemAllocPolicy; ClearPolicy = js::detail::HashTableClearPolicy<true>]’
 3:12.75 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:204:57:   required from ‘void js::HashMap<Key, Value, HashPolicy, AllocPolicy>::clear() [with Key = js::gc::Cell*; Value = long unsigned int; HashPolicy = js::PointerHasher<js::gc::Cell*>; AllocPolicy = js::SystemAllocPolicy]’
 3:12.75 /home/allstars/src/gcc8/js/src/gc/Zone.h:674:35:   required from here
 3:12.75 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:872:15: warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘class js::detail::HashTableEntry<js::HashMapEntry<js::gc::Cell*, long unsigned int> >’ with no trivial copy-assignment [-Wclass-memaccess]
 3:12.75          memset(table, 0, sizeof(*table) * capacity);
 3:12.75          ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 3:12.75 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:794:7: note: ‘class js::detail::HashTableEntry<js::HashMapEntry<js::gc::Cell*, long unsigned int> >’ declared here
 3:12.75  class HashTableEntry

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Yoshi Huang [:allstars.chh] from comment #1)
This looks like it should work.  I'll try and install GCC 8 and see what's happening.

BTW, it would be simpler not to have HashTableClearPolicy as a parameter for the main HashTable.  You can just have a ClearPolicy class and select the right one when you call the clear method.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1416666-hash-table-clear

Here's a simplified version of your patch.  I tested this with GCC 8 and it fixes the problem warnings around UniquePtr, but still generates tons of warnings for other types where we do want to use memcpy.

This is complaining that HashTableEntry has no trivial copy-assignment, and this is correct because we deleted that method.  I'm not sure what the best solution is here.

Comment 4

[Jeff Walden [:Waldo]]

Just so I'm clear what we're doing here -- the problem basically is that the clear function *always* includes memsets (even if unreachable by guarding by constant expression, in reality).  And appearance even in unreachable dead code triggers the warning.  So this just ensures the memset doesn't even appear in compiled code, when it won't be called?

If so, this looks like a prime use case for |if constexpr|, if only our compiler requirements supported it.  Maybe worth a comment noting we should make the switch when possible.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #4)
> Just so I'm clear what we're doing here -- the problem basically is that the
> clear function *always* includes memsets (even if unreachable by guarding by
> constant expression, in reality).  And appearance even in unreachable dead
> code triggers the warning.  So this just ensures the memset doesn't even
> appear in compiled code, when it won't be called?
> 
Yes.

> If so, this looks like a prime use case for |if constexpr|, if only our
> compiler requirements supported it.  Maybe worth a comment noting we should
> make the switch when possible.
Will check this.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) (PTO until 20th Nov) from comment #3)
> Created attachment 8928547 [details] [diff] [review]
> bug1416666-hash-table-clear
> 
> Here's a simplified version of your patch.  I tested this with GCC 8 and it
> fixes the problem warnings around UniquePtr, but still generates tons of
> warnings for other types where we do want to use memcpy.
> 
I'll file seperate bugs for other warnings.
Yes, the patch is easier, but do you know there's still a warning for HashTableEntry<js::HashMapEntry<js::gc::Cell*, long unsigned int> >, like what I mentioned in comment 1?
This warning is also showing in this patch.

3:04.98 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h: In instantiation of ‘static void js::detail::ClearHelper<true>::clear(js::detail::HashTableEntry<T>*, uint32_t) [with T = js::HashMapEntry<js::gc::Cell*, long unsigned int>; uint32_t = unsigned int]’:
 3:04.98 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:1696:53:   required from ‘void js::detail::HashTable<T, HashPolicy, AllocPolicy>::clear() [with T = js::HashMapEntry<js::gc::Cell*, long unsigned int>; HashPolicy = js::HashMap<js::gc::Cell*, long unsigned int, js::PointerHasher<js::gc::Cell*>, js::SystemAllocPolicy>::MapHashPolicy; AllocPolicy = js::SystemAllocPolicy]’
 3:04.98 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:200:57:   required from ‘void js::HashMap<Key, Value, HashPolicy, AllocPolicy>::clear() [with Key = js::gc::Cell*; Value = long unsigned int; HashPolicy = js::PointerHasher<js::gc::Cell*>; AllocPolicy = js::SystemAllocPolicy]’
 3:04.98 /home/allstars/src/gcc8/js/src/gc/Zone.h:674:35:   required from here
 3:04.98 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:873:15: warning: ‘void* memset(void*, int, size_t)’ clearing an object of type ‘class js::detail::HashTableEntry<js::HashMapEntry<js::gc::Cell*, long unsigned int> >’ with no trivial copy-assignment [-Wclass-memaccess]
 3:04.98          memset(table, 0, sizeof(*table) * capacity);
 3:04.98          ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Yoshi Huang [:allstars.chh] from comment #6)
Yes this is still warning about not being able to memset HashTableEntry memory.  It complains that HashTableEntry no has trivial copy-assignment, which is true.  But I don't think we can easily change that because it uses AlignedStorage2.  So I don't know how we are going to fix these warnings.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Andi, do you think you could help with that?

Comment 9

[Andi [:andi]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Andi, do you think you could help with that?
Sure we've had this kind of issues in the past.

Comment 10

[Andi [:andi]]

I looked other this and my result is that there is no direct way on how circumventing AlignedStorage2 when dealing with IsPod<js::detail::HashTableEntry<T> >. I think another approach on this matter would be the usage of if constexpr(..) but we are only compiling for the moment with dialect c++14. 
Leaving this aside, yest this warning is useful but in the current context I'm not sure it adds something useful, or that it raises an important matter that should be handled quickly. So my take on this would be just to ignore this warning for the moment.

Comment 11

[Andi [:andi]]

Do you think it's acceptable to drop altogether the memset and clear each element from the Entity?

Comment 12

[Jeff Walden [:Waldo]]

Okay, it's 01:35 where I am and I am well past the end of a workday and this analysis really demands a double-check, but:

Why are we memsetting |table| if we're in the act of *clearing* the hashtable such that no entries in it are valid?

We have to do it for the non-POD case because there may be destructors to run and things to clean up.

But in the POD case, there...nothing there to clean up.  memsetting does *nothing that we should observe*.  We shouldn't be looking into the nothing and seeing anything there, because we're *clearing* the table.  |entryCount = 0| should mean we never look into this allocated-but-garbaged table.  Right?

So I think (he said, at now 01:49 after distractions and with clearly more mental sense):

* we should remove the memset-arm
* we should only do the walk/clear for non-POD
* after the walk/clear, we should mark the memory undefined for Valgrind/ASan (#include "mozilla/MemoryChecking.h" if needed)

  MOZ_MAKE_MEM_UNDEFINED(table, sizeof(*table) * capacity());

Now somebody tell me why my sleep-deprived musings are wrong.  :-)

Comment 13

[Jeff Walden [:Waldo]]

Also maybe that was what comment 11 said?  I am not entirely sure how to parse that comment, in my sleep-deprived-addled state.

Comment 14

[Andi [:andi]]

Attachment: Bug 1416666 - Avoid doing a memset on a non-POD structure.

Review commit: https://reviewboard.mozilla.org/r/241650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241650/

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8973146 [details]
Bug 1416666 - Avoid doing a memset on a non-POD structure.

https://reviewboard.mozilla.org/r/241650/#review247664

::: js/public/HashTable.h
(Diff revision 1)
>    public:
>      void clear()
>      {
> -        if (mozilla::IsPod<Entry>::value) {
> -            memset(table, 0, sizeof(*table) * capacity());
> -        } else {

You should keep around the

    if (!mozilla::IsPod<Entry>::value) {
        ...
    }

guard around this.

There's nothing _wrong_ with doing the clearing if POD, *but* clearing *does* have effects beyond just calling the constructor: it sets a hashNumber in all the affected entries, that theoretically is observable.  It's doubtful the compiler will be smart enough to figure out that it can just not do anything here -- the make-mem-undefined is *not* enough, because unless you're compiling with asan/valgrind on, it's just a no-op statement.

Comment 16

[Andi [:andi]]

Comment on attachment 8973146 [details]
Bug 1416666 - Avoid doing a memset on a non-POD structure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241650/diff/1-2/

Comment 17

[Andi [:andi]]

I haven't landed this since I did a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=846041df12df6d598c38de85429d1aba469a677f

The thing that scares me here is the huge number of js tests that started to fail with this patch. Clearly something is happening here.

For example linux debug: https://treeherder.mozilla.org/logviewer.html#?job_id=177439451&repo=try

For example we see almost all of the XPCShellTestsTests tests fail. 

Jeff, do you have any idea on why this might happen?

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Patch that actually should work

So my sleep-addled query was actually wrong (real shocker, that).  It's been awhile since I looked at this, but |table| *does* have to be partially valid even for a cleared hashtable.

Thing is, |entryCount| tracks -- in fast-to-check manner -- the number of entries in the hashtable.  But to actually enumerate entries, we have to loop through all of |table|, checking for entries that are actually live.  A live entry is indicated by a zero |hash| in the entry.  The |memset| would properly zero that; removing the memset will not.

It's not entirely clear whether a memset that overwrites a lot of stuff but is maybe simpler, is faster than compiler-generated likely-SIMD code that zeroes out *just* |hash| fields in all the entries.  But I am going to guess that SIMD is good enough.  Until someone spends time benchmarking this and gathering proof, we should just do the simple and stupid thing: don't distinguish POD and non-POD, and know that the compiler is going to recognize that |mem.addr()->~T()| is a no-op when T is trivial.  So with POD, the loop should degenerate to just zeroing |hash| at consistent offset, and SIMD will eat that up, and it can't be *that* different from the memset in performance (if it is at all).

This is roughly close enough to the patch posted here that I am just going to land this as already reviewed.  Will tack it onto my next push of things.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b1051804d1
Avoid doing a memset on a non-POD structure.  r=jwalden

Comment 20

[Natalia Csoregi [:nataliaCs]]

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

Comment 21

[Jan de Mooij [:jandem]]

YG_Akwafina29, please stop modifying bugs.