1415982 - incoming breakage from bug 1415980 (fix some issues with how we do hash keys)

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

Bug 1415980 introduces a few things that will almost certainly break Thunderbird builds.  If you have any classes that inherit from PLDHashEntryHdr (or hash keys like those from nsHashKeys.h), you need to make sure that they do not implement a copy constructor.  If they don't declare one, that's fine; if they do declare one, it'll need to be replaced with a move constructor.  The first patch in bug 1415980 has lots of examples of how things can be fixed, and I'm happy to answer any questions as well.

Comment 1

[Jorg K (CEST = GMT+2)]

Since bug 1415980 is moving again, it's time to return to this bug here. We derive a few classes from PLDHashEntryHdr :
https://dxr.mozilla.org/comm-central/search?q=regexp%3Apublic.*PLDHashEntryHdr&redirect=false
mailnews/db/msgdb/public/nsMsgDatabase.h
385 struct MsgHdrHashElement : public PLDHashEntryHdr {
397 struct RefHashElement : public PLDHashEntryHdr {
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
73 struct BaseToken : public PLDHashEntryHdr

How do I check for a copy constructor? Do your changes in bug 1415980 produce a compile error?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jorg K (GMT+1) from comment #1)
> Since bug 1415980 is moving again, it's time to return to this bug here. We
> derive a few classes from PLDHashEntryHdr :
> https://dxr.mozilla.org/comm-central/search?q=regexp%3Apublic.
> *PLDHashEntryHdr&redirect=false
> mailnews/db/msgdb/public/nsMsgDatabase.h
> 385 struct MsgHdrHashElement : public PLDHashEntryHdr {
> 397 struct RefHashElement : public PLDHashEntryHdr {
> mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
> 73 struct BaseToken : public PLDHashEntryHdr
> 
> How do I check for a copy constructor?

For some class T, if the class has a method `T(const T&)`, then it has an explicitly declared copy constructor, which you'll need to switch to a move constructor.  I suppose you might see `T(T&)` methods as well, which you'd also need to switch, but this should be unlikely.  Classes can also have *implicitly* declared copy constructors, but...

> Do your changes in bug 1415980 produce a compile error?

...you should see compiler errors for such classes, and I believe you should also see compile errors for classes that have explicitly declared copy constructors.

Comment 3

[Jorg K (CEST = GMT+2)]

I've applied
https://hg.mozilla.org/integration/mozilla-inbound/rev/9035ff3757ac
and get heaps of compile errors. So let's see how to fix them.

Mostly:
 1:38.58 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\PLDHashTable.h(52): error C2580: 'PLDHashEntryHdr::PLDHashEntryHdr(PLDHashEntryHdr &&)': multiple versions of a defaulted special member functions are not allowed

That comes from here:
https://hg.mozilla.org/integration/mozilla-inbound/diff/9035ff3757ac/xpcom/ds/PLDHashTable.h

Nathan, what is that error trying to tell me?

Comment 4

[Nathan Froyd [:froydnj]]

It's hard to say without seeing the contextual code, i.e. what inherits from PLDHashEntryHdr there.

Comment 5

[Jorg K (CEST = GMT+2)]

Well, the first thing that doesn't compile is this:
https://dxr.mozilla.org/comm-central/source/mailnews/jsaccount/src/DelegateList.cpp
https://dxr.mozilla.org/comm-central/source/mailnews/jsaccount/src/DelegateList.h

Comment 6

[Nathan Froyd [:froydnj]]

Well, that's using nsCStringHashKey, which has been converted to the New World Order.  So either that's not the context that the original error message was occurring from, or there are other problems in Thunderbird that aren't showing up in the Firefox build.  I am inclined towards the former.

Please paste the *entire* error next time.  I'm guessing what might be happening is that you're seeing intermixed output from multiple compilation processes, and that's making the file that's actually causing the error not clear.

Comment 7

[Jorg K (CEST = GMT+2)]

 1:32.70 DelegateList.cpp
 1:32.71 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\PLDHashTable.h(52): error C2580: 'PLDHashEntryHdr::PLDHashEntryHdr(PLDHashEntryHdr &&)': multiple versions of a defaulted special member functions are not allowed
 1:32.71 mozmake[4]: *** [c:/mozilla-source/comm-central/config/rules.mk:1054: DelegateList.obj] Error 2
 1:32.71 mozmake[3]: *** [c:/mozilla-source/comm-central/config/recurse.mk:74: comm/mailnews/jsaccount/src/target] Error 2

I'm pretty sure that it's DelegateList.cpp that fails to compile.

So what's the "new world order" for nsCStringHashKey?

BTW, the compiler is complaining about this block of code, that's pretty clear:
 struct PLDHashEntryHdr
 {
+  PLDHashEntryHdr() = default;
+  PLDHashEntryHdr(const PLDHashEntryHdr&) = delete;
+  PLDHashEntryHdr(const PLDHashEntryHdr&&) = delete;
+  PLDHashEntryHdr& operator=(const PLDHashEntryHdr&) = delete;
+  PLDHashEntryHdr(PLDHashEntryHdr&&) = default;
+  PLDHashEntryHdr& operator=(PLDHashEntryHdr&&) = default;

As I wrote in bug 1415980, it could be a compiler error, which won't show in automation which is using clang-cl. I could be wrong at 12:05 AM when it's time to retire.

Comment 8

[Jorg K (CEST = GMT+2)]

Aceman compiled on Linux and he sees issues caused by TB code, like:
                  *tp++ = *(static_cast<Token*>(e.nextToken()));
                                                              ^
/var/NVME/TB-hg/mozilla/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:78:8: note: ‘Token& Token::operator=(const Token&)’ is implicitly deleted because the default definition would be ill-formed:
  struct Token : public BaseToken {
         ^~~~~

To be continued when the M-C patch can be compiled on MSVS C++ ;-)

Comment 9

[Ben Campbell]

Attachment: bug_1415982_msgdb_and_bayesian.patch

Fix for the new PLDHashEntryHdr-related compile errors in:
  comm/mailnews/db/msgdb/src/nsMsgDatabase.cpp
  mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp

nsBayesianFilter.cpp is a little problematic - it copies out PLDHashEntryHdr-derived Token objects, detached from any PLDHashtable. So their PLDHashEntryHdr::mKeyHash values will be undefined.
I don't _think_ this is a problem in the context in which nsBayesianFilter.cpp uses them, but it seems like maybe there should be a more elegant solution.

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks Ben. We can always ask Nathan Froyd [:froydnj] for feedback. For now, bug 1415980 has been backed out since the resulting code doesn't compile with MSVS C++ 15.6.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: bug_1415982_msgdb_and_bayesian.patch (with white-space fixes)

I've switched to clang-cl locally now, so I was able to compile this.

Nathan, could you please give us your feedback on this, if that's possible without much context.

What is your plan for bug 1415980? Wait until MSVS C++ is no longer used? Or gets upgraded to 15.8? No surprise your patch already partly doesn't apply any more.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 9002245 [details] [diff] [review]
bug_1415982_msgdb_and_bayesian.patch  (with white-space fixes)

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

I plan to commit bug 1415980 when we switch over to a version of MSVC that isn't busted, or when we drop MSVC support entirely.

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +81,5 @@
>                              // object for the first trait for this token
> +                            //
> +    // Copy assignment to support Tokenizer::copytokens()
> +    // NOTE: PLDHashEntryHdr will be left undefined!
> +    Token& operator= (const Token& t) {

This is pretty ugly, but I guess it works?  I think the more elegant solution would be to have:

Token clone(const Token&) {
  Token t;
  // Initialize fields, or have a Token constructor to use above.
  return t;
}

and use that in copytokens.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 9035ff3757ac.patch - M-C patch from bug 1415980 rebased on 2018-08-18

(In reply to Nathan Froyd [:froydnj] from comment #12)
> This is pretty ugly, but I guess it works?  I think the more elegant
> solution would be to have:
I hope, Ben can revise his patch. Last time I tried, this M-C patch still applied.

Comment 14

[Jorg K (CEST = GMT+2)]

I forgot to say: Many thanks, Nathan, for taking the time to look at this for us!

Comment 15

[Ben Campbell]

Attachment: hash_breakage.patch

New version of my previous patch, incorporating Nathan's suggestions. Uses an explicit Token::clone() function instead of an implicitly half-assed copy operator.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003731 [details] [diff] [review]
hash_breakage.patch

Nathan, could you please take another look. This looks like what you suggested.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c33ef0679b50
Port bug 1415980: cope with PLDHashEntryHdr no longer being copyable. f=froydnj, rs=jorgk