RFR [XS]: 8236709: struct SwitchRange in HS violates C++ One Definition Rule - was RE: struct SwitchRange and C++ One Definition Rule [-Wodr]

[David Holmes]

Hi Matthias,

On 7/01/2020 7:27 pm, Baesken, Matthias wrote:
> Hello, please review  this small fix for an issue  that I was running into  when experimenting  with  gcc8  and   the -flto compiler flag .
> When building with those flags,  the gcc8 warns that  the  SwitchRange   classes  in HS code   violate the C++ One Definition Rule .
> So I renamed one of those 2  SwitchRange   classes .

Could you instead put the ./share/opto/parse2.cpp version inside an 
anonymous namespace? It seems to me that both SwitchRange classes are 
intended for use in a single compilation unit, so if we can make that 
more obvious that seems better than solving the name clash.

Otherwise I have to wonder whether you could rename the C1 version as 
you have in the hpp file (to avoid the global name clash) but add a 
typedef to allow the cpp file (and latter part of the header) to be 
unchanged? Also minor nit:

   class Invoke;
! class SwitchRangeC1;
   class LIRItem;

The forward declaration seems unnecessary.

Thanks,
David
-----

>   Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8236709
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8236709.0/
> 
> 
> Thanks, Matthias
> 
> 
>>> Hello,  when  experimenting  with  gcc8  and   the -flto compiler flag I was running into these  warnings  in the c1  coding :
>>>
>>> /open_jdk/jdk_3/jdk/src/hotspot/share/c1/c1_LIRGenerator.hpp:50:7:
>>> warning: type 'struct SwitchRange' violates the C++ One Definition Rule [-
>>> Wodr]
>>> class SwitchRange: public CompilationResourceObj {
>>>         ^
>>> /open_jdk/jdk_3/jdk/src/hotspot/share/opto/parse2.cpp:319: note: a
>>> different type is defined in another translation unit
>>> class SwitchRange : public StackObj {
>>>
>>>
>>>
>> /usr/work/d040975/open_jdk/jdk_3/jdk/src/hotspot/share/c1/c1_LIRGener
>>> ator.hpp:52:7: note: the first difference of corresponding definitions is field
>>> '_low_key'
>>>     int _low_key;
>>>         ^
>>> /open_jdk/jdk_3/jdk/src/hotspot/share/opto/parse2.cpp:321: note: a
>>> field with different name is defined in another translation unit
>>>     jint _lo;                     // inclusive lower limit
>>>
>>>
>>> Do  you think this should be fixed ( renaming  one SwitchRange  ) ?
>>>
>>>
>>> Martin suggested that  even without  flto added   it could be problematic .
>>
>> Yes, please file a bug and let's get this fixed quickly.  This sort of thing can
>> lead
>> to really weird looking problems, like using the same vtable (which one
>> picked
>> “at random”) for instances of both classes.
>