Closed Bug 1415982 Opened 2 years ago Closed Last year

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

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: froydnj, Assigned: benc)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Summary: incoming breakage from bug 1415980 → incoming breakage from bug 1415980 (fix some issues with how we do hash keys)
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?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
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?
Flags: needinfo?(nfroyd)
It's hard to say without seeing the contextual code, i.e. what inherits from PLDHashEntryHdr there.
Flags: needinfo?(nfroyd)
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.
 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.
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++ ;-)
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.
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.
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.
Attachment #8998407 - Attachment is obsolete: true
Attachment #9002245 - Flags: feedback?(nfroyd)
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.
Attachment #9002245 - Flags: feedback?(nfroyd) → feedback+
(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.
I forgot to say: Many thanks, Nathan, for taking the time to look at this for us!
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 on attachment 9003731 [details] [diff] [review]
hash_breakage.patch

Nathan, could you please take another look. This looks like what you suggested.
Attachment #9003731 - Flags: feedback?(nfroyd)
Assignee: nobody → benc
Status: NEW → ASSIGNED
Attachment #9003731 - Flags: feedback?(nfroyd) → feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c33ef0679b50
Port bug 1415980: cope with PLDHashEntryHdr no longer being copyable. f=froydnj, rs=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.