RFR; 8077420: Build failure with SS12u4

Tue Apr 14 08:50:08 UTC 2015

On 2015-04-14 04:04, David Holmes wrote:
> Hi Stefan,
>
> On 13/04/2015 10:52 PM, Stefan Karlsson wrote:
>> Hi David,
>>
>> On 2015-04-13 14:25, David Holmes wrote:
>>> Hi Stefan,
>>>
>>> On 13/04/2015 9:57 PM, Stefan Karlsson wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for a build failure that was encountered during
>>>> the Solaris Studio compiler upgrade.
>>>
>>> So what was it about the compiler upgrade that exposed this ??
>>
>> I'm not sure, but somehow the function wasn't made available to the
>> compiler/linker. Maybe there's a combination of the inline keyword and
>> template functions that used to work, but doesn't anymore? I don't have
>> access to a SS12u4 setup, so Erik J had to compile with the patch.
>
> Ok. I'm seeing some other weird stuff coming out in relation to 
> SS12u4. :)
>
>>>
>>>> Two files were missing an include of stack.inline.hpp, which is needed
>>>> to get the definition of default_segment_size(). This function is used
>>>> in the stack.hpp, to provide the default value to the constructor. 
>>>> This
>>>> forces all users of Stacks with the default segment size, to have to
>>>> include stack.inline.hpp. The default_segment_size() implementation
>>>> always evaluates to the same value, given the template parameters, 
>>>> so I
>>>> chose to setup a constant in the header instead of adding the missing
>>>> stack.inline.hpp files.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8077420/webrev.01/
>>>> https://bugs.openjdk.java.net/browse/JDK-8077420
>>>
>>> This seems okay but I have to say I remain very confused about the
>>> "right" way to deal with .hpp and .inline.hpp files in general.
>>
>> OK. One rule to honor, as far as possible, is to _not_ use functions in
>> inline.hpp files from .hpp files. In this case stack.hpp used a function
>> in stack.inline.hpp.
>
> That seems reasonable given that if you have a .inline.hpp I would 
> expect that all defined functions should be in there. Having some 
> functions in the .hpp and some in the .inline.hpp seems a recipe for 
> total confusion. :)

If a function doesn't "have to" be inlined, I would prefer if it was put 
inside the cpp file instead. Depending on where function definition is 
placed, we "leak" the includes of dependent header files to code that 
doesn't need it, which might cause problems like circular dependencies 
in completely unrelated parts of the code base. If the function 
definition is put in:

1) cpp - Only the cpp file will include the dependent headers.

2) inline.hpp - All includers of the inline.hpp will transitively 
include the dependent headers. Usually, only files calling any of the 
functions in the inline.hpp should include the inline.hpp files. 
Therefore, we don't want to call functions in inline.hpp files from hpp 
files, since that will spread the dependent includes to unrelated parts 
of the code base.

3) hpp - All includers of the hpp files will transitively include the 
dependent headers. Whereas inline.hpp files should only be included if 
any of the functions in the file are used, hpp files are needed to get 
typedefs, macros, class structure, etc. so they are included from more 
often. So, placing the function definition in the hpp file will most 
likely spread the dependent includes even more.

I thinks we should strive to put function definition into cpp file, 
unless they are performance critical, then we should consider placing 
them in inline.hpp files and take the extra maintenance cost of having 
the inline.hpp file. Putting the definition in the hpp file should be 
avoided, unless it's small functions like getter/setter, and that those 
functions don't use functions defined in other files. This also needs to 
be considered when adding code to an existing function in a hpp file. 
For example, don't add assert(Univese::heap()->promotion_should_faile(), 
"sanity"), to a hpp file, since that will bring in the dependent 
includes assert.hpp, universe.hpp, collectedHeap.inline.hpp.

>
>> Maybe there's a more authoritative place that describes the "right way",
>> but I think this post is worth reading and describes it well enough:
>> http://www.cplusplus.com/forum/articles/10627/
>
> It doesn't really address the .inline.hpp, as it never states 
> when/where to include the .inline.hpp file - there is even some 
> discussion in the comments about the problems relating to wanting 
> functions inlined if not in the included header, but no resolution.

You're right that it doesn't address where to include the inline.hpp 
file, and it actually suggests that the inline.hpp file should be 
included at the end of the hpp file. I misread that part. :(

> He also says that including the .hpp in the .inline.hpp is harmless 
> but unnecessary - which suggest he expected client code to include 
> both of them (whereas I think what has been advocated in hotspot 
> lately is for client code to include the .inline.hpp and for the the 
> .inline.hpp to include the .hpp).

I think he says it's unnecessary since he is including the inline.hpp 
into the hpp file. Whereas we (should) only include inline.hpp files in 
cpp files or other inline.hpp files.

Including only the inline.hpp file and not both hpp and inline.hpp files 
are just a convention that I like to keep the include line lengths down 
a bit. If it's causing confusions then it might be better if we skip 
that convention. As an example, the include list in iterator.inline.hpp 
would then go from:

#include "classfile/classLoaderData.hpp"
#include "memory/iterator.hpp"
#include "oops/klass.hpp"
#include "oops/instanceKlass.inline.hpp"
#include "oops/instanceMirrorKlass.inline.hpp"
#include "oops/instanceClassLoaderKlass.inline.hpp"
#include "oops/instanceRefKlass.inline.hpp"
#include "oops/objArrayKlass.inline.hpp"
#include "oops/typeArrayKlass.inline.hpp"
#include "utilities/debug.hpp"

to:

#include "classfile/classLoaderData.hpp"
#include "memory/iterator.hpp"
#include "oops/klass.hpp"
#include "oops/instanceKlass.hpp"
#include "oops/instanceKlass.inline.hpp"
#include "oops/instanceMirrorKlass.hpp"
#include "oops/instanceMirrorKlass.inline.hpp"
#include "oops/instanceClassLoaderKlass.hpp"
#include "oops/instanceClassLoaderKlass.inline.hpp"
#include "oops/instanceRefKlass.hpp"
#include "oops/instanceRefKlass.inline.hpp"
#include "oops/objArrayKlass.hpp"
#include "oops/objArrayKlass.inline.hpp"
#include "oops/typeArrayKlass.hpp"
#include "oops/typeArrayKlass.inline.hpp"
#include "utilities/debug.hpp"

>
> I did like the simple rules regarding forward declarations versus 
> include dependencies. My C++ introduction was in an environment where 
> forward declarations were seen as a hack to deal with circular 
> dependencies, rather than being a first class mechanism for 
> introducing type names.

They are a nice way to lower the include dependency in header files. 
Unfortunately, enums can't be forward declared in C++03.

Thanks,
StefanK

>
> Cheers,
> David
>
>> Thanks,
>> StefanK
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Tested with JPRT, and built with SS12u4.
>>>>
>>>> Thanks,
>>>> StefanK
>>