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

[coleen.phillimore at oracle.com]

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!
>
> 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?

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
>
>
>