209622 - nsTHashtable cause bustage on Solaris with F6U2

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Yuan]

The fix of bug 202080 cause this bustage, but it more likes a compiler bug - 
the same source code can be smoothly compiled using Sun's new compiler K2 
(5.5). I have a workaround fix for this.

Comment 1

[Kyle Yuan]

Attachment: the workaround fix

Comment 2

[Roland Mainz]

Is Sun Workshop 7+patches affected by this issue, too - or is it only Sun
Workshop 6 Update 2 which is on drugs here ?

Comment 3

[Roland Mainz]

BTW: last time I asked (few weeks ago) the Sun Workshop compiler on "nebiros"
was horrible behind the current patch status (see
http://access1.sun.com/patch.public/cgi-bin/show_list.cgi/wrk/Forte_HPC_6u2_SPARC).
If the issue is fixed in Workshop 7 then it is likely that applying compiler
patches will solve this issue.

Comment 4

[Benjamin Smedberg]

Kyle, if you remove the "using" declarations entirely (instead of making them
public), does that solve the problem? if so, I would prefer to add a configure
test for this rather than doing system-specific hacks.

I feel like I've been trading off platforms: fix that, break this, fix that,
break yet another ;)

Comment 5

[Donnie Cranford]

This bug is happening on Forte 7 as well, and im fully up to date on patches

I have yet to try the patch...will let you know how it goes

Comment 6

[Roland Mainz]

Comment on attachment 125791 [details] [diff] [review]
the workaround fix

Two comments on the workaround patch:
1. Please replace the |ifdef SOLARIS| with |ifdef __SUNPRO_CC| to make the
workaround specific to the Sun Workshop compiler
2. Please put a comment in both places why this was done incl. a reference to
this bug

Comment 7

[Roland Mainz]

Comment on attachment 125791 [details] [diff] [review]
the workaround fix

r=[e-mail] if you fix the issues listed
above...

... bz - can you sr= it, please ?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125791 [details] [diff] [review]
the workaround fix

I don't know anything about this code, and in any case this patch needs review
from the owner of this code (bsmedberg) or at least an xpcom owner/peer.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(When I first saw the error I was expecting this to be the issue in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10152 but it clearly isn't.  I'm
only mentioning this to make my reaction on IRC clearer.)

The code that it's complaining about was, it looks like, put in for bug 201407.
 I think this is the only use of HAVE_CPP_AMBIGUITY_RESOLVING_USING for the
inherited template scope issue -- it's really not ambiguity resolution, but
rather introduction into the scope.  Perhaps we need a different autoconf test
for this use?  Which leads to the question:

Does the code in question compile if the |using| declarations are removed?

Comment 10

[Kyle Yuan]

Attachment: get rid of |using| in HashTable classes declarations

bsmedberg, dbaron
yes, removing |using| does fix the compile problem. so the patch should like
this. I've verified it on Solaris, RedHat 8 & Windows 2000.

Comment 11

[Benjamin Smedberg]

Comment on attachment 125870 [details] [diff] [review]
get rid of |using| in HashTable classes declarations 

But that regresses bug 201407, which is why the "using" declarations were
inserted in the first place. I'm writing an autoconf test for this, which I
will post shortly.

Comment 12

[Kyle Yuan]

Comment on attachment 125870 [details] [diff] [review]
get rid of |using| in HashTable classes declarations 

okay, I'd like to test your patch on Solaris.

Comment 13

[Roland Mainz]

Just curios: Is nsTHashtable thought as a public interface which may be used by
3rd-party components ?

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 125870 [details] [diff] [review]
get rid of |using| in HashTable classes declarations 

This wasn't even what I meant -- I wanted to know whether it would compile with
the whole using declaration removed, not just the word |using|.  I'm not sure
what this does.

Comment 15

[Kyle Yuan]

sorry for the misunderstanding. removing these lines:

diff -u -r3.6 nsBaseHashtable.h

-#ifdef HAVE_CPP_AMBIGUITY_RESOLVING_USING
-  using nsTHashtable<nsBaseHashtableET<KeyClass,DataType> >::mTable;
-#endif
-

also fixes the problem on Solaris.

Comment 16

[Benjamin Smedberg]

Attachment: configure test

OK, I just took the example from
http://gcc.gnu.org/onlinedocs/gcc/Name-lookup.html and pasted it as a configure
test. Please note that since I just committed bug 208437, you will need to cvs
up before this patch will apply.

Comment 17

[Benjamin Smedberg]

Comment on attachment 125901 [details] [diff] [review]
configure test

Oops, please ignore the bit in configure.in about chrome-format=symlink that's
for a different bug.

Comment 18

[Raja Harinath]

The impact of using the 'using' approach seems farreaching and heavyweight.

I think that using the 'this->mTable' idiom (see Bug #201407) would have made it
a localized fix that has a better chance of working as is.  IIRC, the reason it
wasn't used was aesthetic.  But I think having a configure test for this usage
of 'using' is going overboard.

Comment 19

[Benjamin Smedberg]

dbaron: this this->mTable workaround is OK with me as long as it producese the
same code (assembly-wise). Since there are no virtual functions involved I can't
imagine that would be a problem. What do you think?

jkeiser: if we go that route, we'll need to update those C++ portability docs I
sent you.

Comment 20

[Bradley Baetz (:bbaetz)]

If gcc -S doesn't give you the same code for using vs ->, file a gcc bug, I think.

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 125901 [details] [diff] [review]
configure test

I'm not a big fan of this test because I prefer to test for bugs, and default
to correct code unless the bugs are present.

As comment 18 suggests, maybe |this->mTable| would be better?  I doubt there
would be differences in generated assembly.

Comment 22

[Raja Harinath]

By definition, there can be no problem with 'this->' to refer to a class member.
Whenever a bareword 'foo' resolves to a particular member, 'this->foo' should
resolve to the same.  The dynamic type of 'this' is the same as it's static
type: a pointer to the class where the current method is being defined.  So, any
virtual lookup will agree with the usual namespace lookup of the corresponding
bareword.

In terms of codegen, in practice, every C++ ABI I know passes the 'this' pointer
as a hidden parameter to every non-static method.  Any member reference
(bareword or 'this'-adorned) will be compiled down to an offset from this
pointer -- so the code generated is likely the same.

Comment 23

[Benjamin Smedberg]

Attachment: use this-> instead of a configure test

Uses this-> instead of a configure test. This will compile on gcc3.4, but it
gives link-time errors that I'm pretty sure are a compiler bug. I will file a
gcc bug when I get the chance to investigate.

Comment 24

[Jim Brown]

OS: OSF1  Version: trunk  Hardware: DEC  System: Tru64 V5.1B, C++ v6.5

I am getting 'invalid base class' for 'using nsTHashtable<EntryType>::mTable;'
in nsBaseHashtable.h at line 298 since the hash table change went in a few days ago.
It is not just Solaris.

Comment 25

[Raja Harinath]

The gcc3.4 link problem is probably the same as 205407, which has been forwarded
to http://gcc.gnu.org/PR10804.

Comment 26

[Alec Flett]

Comment on attachment 126019 [details] [diff] [review]
use this-> instead of a configure test

/me shrugs
sr=alecf - it seems safe to me - I mean the generated code should be more or
less identical.

Comment 27

[Alec Flett]

Comment on attachment 125791 [details] [diff] [review]
the workaround fix

yay solaris! sr=alecf I suppose.. so there's no good way to use a configure
test instead of relying on compiler-specific #defines?

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 126019 [details] [diff] [review]
use this-> instead of a configure test

>Index: configure.in

If you're removing this unused autoconf test, you should also remove the
corresponding |#define| for Windows in nscore.h.

>Index: xpcom/ds/nsBaseHashtable.h
>@@ -184,11 +184,11 @@
>   {
>     NS_ASSERTION(mTable.entrySize,
>                  "nsBaseHashtable was not initialized properly.");
> 
>     s_EnumReadArgs enumData = { enumFunc, userArg };
>-    return PL_DHashTableEnumerate(NS_CONST_CAST(PLDHashTable*, &mTable),
>+    return PL_DHashTableEnumerate(NS_CONST_CAST(PLDHashTable*, &(this->mTable)),

No need to parenthesize here, or where it's repeated below.

Other than that, r=dbaron.

Comment 29

[Benjamin Smedberg]

fixed on trunk with dbaron's nits. Thanks, all.

Comment 30

[Raja Harinath]

re comment 27:

bsmedberg's previous patch was a good example of using 'configure' to detect
compiler features.

However, since it is trivial to write code that is independent of that feature,
it is cheaper (and IMHO much more cleaner) to write the code that way rather
than bring in the powertools and all the scaffolding necessary.

Comment 31

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 125791 [details] [diff] [review]
the workaround fix

There's no need for this patch to go in.

Comment 32

[Raja Harinath]

Attachment: A couple of places inside NS_ASSERTIONs were missed

Comment 33

[Benjamin Smedberg]

Comment on attachment 126054 [details] [diff] [review]
A couple of places inside NS_ASSERTIONs were missed

oh gah... I'm going to carry over reviews for this. I can't check this in until
later, but anyone with commit access is welcome to check it in.

Comment 34

[Kyle Yuan]

Comment on attachment 126054 [details] [diff] [review]
A couple of places inside NS_ASSERTIONs were missed

okay, I've checked this in.