RFR (L, tedious) 8199809: Don't include frame.inline.hpp and other.inline.hpp from .hpp files

David Holmes david.holmes at oracle.com
Wed Mar 21 13:02:42 UTC 2018


On 21/03/2018 10:39 PM, coleen.phillimore at oracle.com wrote:
> 
> Thomas,
> 
> Thank you for building this.
> 
> On 3/21/18 7:50 AM, Thomas Stüfe wrote:
>> Hi Coleen,
>>
>> I think your patch uncovered an issue.
>>
>> I saw this weird compile error on AIX:
>>
>>    471     54 |   bool is_sigtrap_ic_miss_check() {
>>    471     55 |     assert(UseSIGTRAP, "precondition");
>>    471     56 |     return 
>> MacroAssembler::is_trap_ic_miss_check(long_at(0));
>> ===========================^
>> "/priv/d031900/openjdk/jdk-hs/source/src/hotspot/cpu/ppc/nativeInst_ppc.hpp", 
>> line 56.12: 1540-0062 (S) The incomplete class "MacroAssembler" must 
>> not be used as a qualifier.
>>    471     57 |   }
>>
>> in a number of places. But the definition of class MacroAssembler was 
>> available. So I checked if MacroAssembler was accidentally pulled into 
>> a namespace or a class, and sure enough, your patch caused it to be 
>> defined *inside* the class InterpreterRuntime. See 
>> interpreterRuntime.hpp:
>>
>> class InterpreterRuntime: AllStatic {
>> ...
>>   // Platform dependent stuff
>> #include CPU_HEADER(interpreterRT)
>> ...
>> };
>>
>> which pulls in the content of interpreterRT_ppc.hpp.
>>
>> interpreterRT_ppc.hpp includes
>>
>> #include "asm/macroAssembler.hpp"
>> #include "memory/allocation.hpp"
>>
>> (minus allocation.hpp after your patch)
>>
>> which is certainly an error, yes? We should not pull in any includes 
>> into a class definition.
> 
> Yes, I had this problem with x86 which was very befuddling.  I hate that 
> we include files in the middle of class definitions!

It's a crude but effective way to "extend" a class with platform 
specific code at build time. But it does have constraints.

>>
>> I wondered why this did not cause errors earlier, but the include 
>> order changed with your patch. Before the patch, the error was covered 
>> by a different include order: nothing was really included by 
>> interpreterRT_ppc.hpp, the include directives were noops. I think this 
>> was caused by src/hotspot/share/prims/methodHandles.hpp pulling 
>> frame.inline.hpp and via that path pulling macroAssembler.hpp. With 
>> your patch, it pulls only frame.hpp.
>>
>> One could certainly work around that issue but the real fix would be 
>> to not include anything in files which are included into other 
>> classes. Those are not "real" includes anyway. And maybe add a comment 
>> to that file :)
> 
> I will add a comment to all of these like:
> 
> // This is included in the middle of class Interpreter.
> // Do not include files here.
> 
> Hm so I need to add the #include for macroAssembler.hpp somewhere new 
> like nativeInst_ppc.hpp or does just removing it from 
> interpreterRT_ppc.hpp fix the problem?

Whatever code is in the included platform specific header still needs to 
ensure the definitions that it needs have been included. If those are 
shared files then you may just be able to move them into the shared cpp 
file, but any platform specific headers must still be included in the 
platform specific headers.

David
-----

> thanks,
> Coleen
> 
> 
>>
>> Thanks, Thomas
>>
>>
>>
>>
>>
>> On Wed, Mar 21, 2018 at 11:41 AM, Thomas Stüfe 
>> <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>>     Hi Coleen,
>>
>>     linuxs390 needs this:
>>
>>     - .../source $ hg diff
>>     diff -r daf3abb9031f src/hotspot/cpu/s390/interpreterRT_s390.cpp
>>     --- a/src/hotspot/cpu/s390/interpreterRT_s390.cpp      Wed Mar 21
>>     08:37:04 2018 +0100
>>     +++ b/src/hotspot/cpu/s390/interpreterRT_s390.cpp      Wed Mar 21
>>     11:12:03 2018 +0100
>>     @@ -65,7 +65,7 @@
>>      }
>>
>>      // Implementation of SignatureHandlerGenerator
>>     
>> -InteprerterRuntime::SignatureHandlerGenerator::SignatureHandlerGenerator( 
>>
>>     
>> +InterpreterRuntime::SignatureHandlerGenerator::SignatureHandlerGenerator( 
>>
>>          const methodHandle& method, CodeBuffer* buffer) :
>>     NativeSignatureIterator(method) {
>>        _masm = new MacroAssembler(buffer);
>>        _fp_arg_nr = 0;
>>
>>     (typo). Otherwise it builds fine.
>>
>>     I'm getting build errors on AIX which are a bit more complicated,
>>     still looking..
>>
>>     Thanks, Thomas
>>
>>
>>     On Wed, Mar 21, 2018 at 1:08 AM, <coleen.phillimore at oracle.com
>>     <mailto:coleen.phillimore at oracle.com>> wrote:
>>
>>         Summary: Remove frame.inline.hpp,etc from header files and
>>         adjust transitive includes.
>>
>>         Tested with mach5 tier1 on Oracle platforms: linux-x64,
>>         solaris-sparc, windows-x64.  Built with open-only sources
>>         using --disable-precompiled-headers on linux-x64, built with
>>         zero (also disable precompiled headers). Roman built with
>>         aarch64, and have request to build ppc, etc.  (Please test
>>         this patch!)
>>
>>         Semi-interesting details:  moved SignatureHandlerGenerator
>>         constructor to cpp file, moved
>>         interpreter_frame_stack_direction() to target specific hpp
>>         files (even though they're all -1), pd_last_frame to
>>         thread_<os_cpu>.cpp because there isn't a
>>         thread_<os_cpu>.inline.hpp file, lastly moved
>>         InterpreterRuntime::LastFrameAccessor into
>>         interpreterRuntime.cpp file, and a few other functions moved
>>         in shared code.
>>
>>         This is the last of this include file technical debt cleanup
>>         that I'm going to do.  See bug for more information.
>>
>>         open webrev at
>>         http://cr.openjdk.java.net/~coleenp/8199809.01/webrev
>>         <http://cr.openjdk.java.net/%7Ecoleenp/8199809.01/webrev>
>>         bug link https://bugs.openjdk.java.net/browse/JDK-8199809
>>         <https://bugs.openjdk.java.net/browse/JDK-8199809>
>>
>>         I'll update the copyrights when I commit.
>>
>>         Thanks,
>>         Coleen
>>
>>
>>
> 


More information about the hotspot-dev mailing list