RFR (L): 8208671: Runtime, JFR, Serviceability changes to allow enabling -Wreorder

[David Holmes]

Hi Thomas,

On 2/08/2018 10:51 PM, Thomas Schatzl wrote:
> Hi,
> 
> On Thu, 2018-08-02 at 22:34 +1000, David Holmes wrote:
>> Hi Thomas,
>>
>> On 2/08/2018 7:09 PM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     there have been several suggestions to try to fix the Hotspot
>>> code to
>>> allow us to enable -Wreorder in the Hotspot sources.
>>
>> Can you enlighten us on what -Wreorder is/does and so what changes
>> you are making please.
> 
>    oh, sorry, my fault. -Wreorder is a compiler switch that checks that
> the order of declaration of members in the initializer lists of
> constructors match the order of declaration in the class.
> 
> C++ always initializes members in the initializer list in the order of
> the declaration in the class, it does not matter whether you specify it
> differently in the initializer list or not.

Wow! is that considered a "feature"??? I can't think why the compiler 
would not simply execute the code in the order you write it. :( If it's 
going to do that then the language should have made it an error to write 
the initializer list any other way. No wonder people just use 
constructor bodies in preference to initializer lists.

> An often overseen error is that you accidentally use other members in
> the initializer list that actually have not been initialized yet,
> although you did the right ordering in the initializer list.
> 
> 
> E.g.
> 
> class X {
>    int b;
>    int a;
> 
>    X() : a(5), b(a) {
>      // actually the content of b is undefined here. As per standard
>      // your program will always initialize b before a.
>    }
> }
> 
> These are somewhat hard to detect errors that might go unnoticed for a
> while otherwise.
> 
> -Wreorder forces you to have b before a in above example in your
> initializer list by giving a warning otherwise, which causes
> compilation failure.

Got it. Thanks. Please add this basic info to the bug report too.

src/hotspot/share/classfile/dictionary.cpp

This looks like leftover code you used to check the order:

+
+   static bool _some_dictionary_needs_resizing;
+   bool _resizable;
+   bool _needs_resizing;
+   void check_if_needs_resize();
+
+   ClassLoaderData* _loader_data;  // backpointer to owning loader
+
   Dictionary::Dictionary(ClassLoaderData* loader_data, int table_size, 
bool resizable)


Otherwise it all seems okay. In some cases it may have been cleaner to 
reorder the variable declarations in the class to have related variables 
adjacent - for example following _name by _name_len, rather than having 
them split by other variables - but that's just a stylistic thing.

Thanks,
David

> Thanks,
>    Thomas
> 
>>
>> Thanks,
>> David
>>
>>> This should make problems due to use-before-initialization much
>>> more
>>> obvious.
>>>
>>> This change fixes initializer lists for the runtime, jfr and
>>> serviceability directories.
>>>
>>> There should be no change in code behavior.
>>>
>>> A later follow-up will enable -Wreorder in the makefiles.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8208671
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8208671/webrev/
>>> Testing:
>>> hs-tier1-4,jdk-tier1-3
>>>
>>> Thanks,
>>>     Thomas
>>>
>