RFR (S) 8002160 : Compilation issue with adlc using latest SunStudio compilers

[John Rose]

On Jun 14, 2013, at 5:26 PM, David Chase <david.r.chase at oracle.com> wrote:

> webrev:
> http://cr.openjdk.java.net/~drchase/8002160/webrev.01/
> 
> bug:
> 1) 12.3 SolarisStudio compilers (and it turns out, also some versions of g++, as well as Eclipse) did not like some of the code in adlc; a reference to overloaded + default-arg'd "swap" failed to resolve.
> 
> 2) Once (1) is cleared, code in c1_LIR.cpp would cause 12.3 iropt (optimizer) to crash.
> 
> fix:
> 1) Add "const" to the overloading, that seems to fix that for SolarisStudio and g++/

That's fine.

> 2) Locate offending function in c1_LIR
> 2a) file bug against Solaris Studio compilers with that as test case
> 2b) modify that function in harmless ways until iropt gets happy.
> 
> (And after the fix, entire new build with SS 12.3, slowdebug and fastdebug, successful compilation, and fastdebug -- the whole new build with SS12.3 - passed the jtreg tests below.

Yikes.  I have two suggestions:  Use fewer declarations, if possible, and mark those few declarations with the bug number and a brief comment about a compiler bug.  That way people won't have to wonder why the code is strange.

For similar "scare comments" see the declaration of ContextStream::next in dependencies.cpp and VMStructEntry::address in vmStructs.hpp.

If declaring 'x' as int not a BasicType is part of the warding spell, then it definitely needs a comment.  Worst case is a compiler on some other platform warns against enum-to-unsigned conversion, and somebody fixes it re-introducing this bug.

Suggest hoisting 'kindfield' next to 'x', and giving them prettier names, like t/k or type/kind.  An extra benefit of hoisting would be removing the extra braces.

OTOH, if the braces are the magic bit (see 6598190) then you don't need to make any new variables.  I suppose you already tried that...  Yikes, again.

> testing:
> This is a sparc-only bug, so I ran jtreg on a Sparc box against compiler and closed/compiler.
> I ran tests twice, once tiered, once tiered stopping at level 1.
> I ran JPRT (old compilers) to be sure that I had not broken anything.
> 
> not 100% sure of the indenting of the switch/case statement after adding local scopes, but it looked slightly non-conforming to start with.

Putting a close brace at the end of a line of code, instead of alone on a following line, seems very unusual to me.  I suggest using a pre-existing switch case as your model for this.  (There are plenty of places where close braces go there, but they are mostly one-liner blocks of code.)

— John