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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Wed Mar 21 16:02:15 UTC 2018



On 3/21/18 11:33 AM, Thomas Stüfe wrote:
> Hi Coleen,
>
> On Wed, Mar 21, 2018 at 1:39 PM, <coleen.phillimore at oracle.com 
> <mailto: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 is annoying. I had errors like this several times over the last 
> years already, especially in the os_xxx_xxx "headers".
>
> All these AllStatic functions would be a perfect fit for C++ namespaces.

Yes, we were going to have namespace os at one point.  namespace 
metaspace would also be nice.
>
>
>>
>>     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.
>
>
> Thank you. This would be also valuable for all the os_<os|cpu>.hpp files.

I didn't change these files so I don't want to update them.  This should 
be another RFE.

Oddly frame_<cpu>.hpp files include synchronizer.hpp but I don't want to 
fix this right now.
>
>
>     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?
>
>
> In this case it seems to be sufficient to remove the include from 
> interpreterRT_ppc.hpp:
>
> - .../hotspot $ hg diff
> diff -r d3daa45c2c8d src/hotspot/cpu/ppc/interpreterRT_ppc.hpp
> --- a/src/hotspot/cpu/ppc/interpreterRT_ppc.hpp Wed Mar 21 12:25:06 
> 2018 +0100
> +++ b/src/hotspot/cpu/ppc/interpreterRT_ppc.hpp Wed Mar 21 16:27:02 
> 2018 +0100
> @@ -26,7 +26,6 @@
>  #ifndef CPU_PPC_VM_INTERPRETERRT_PPC_HPP
>  #define CPU_PPC_VM_INTERPRETERRT_PPC_HPP
>
> -#include "asm/macroAssembler.hpp"
>
>  // native method calls
>
>
> The build went thru afterwards. I assume MacroAssembler was not really 
> needed for linkResolver.cpp.
>
> Thanks, Thomas

This is great.  Can you click on the files and Review it?
Thanks,
Coleen
>
>     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