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

[Thomas Stüfe]

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.

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

Thanks, Thomas






On Wed, Mar 21, 2018 at 11:41 AM, Thomas Stüfe <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> 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
>> bug link https://bugs.openjdk.java.net/browse/JDK-8199809
>>
>> I'll update the copyrights when I commit.
>>
>> Thanks,
>> Coleen
>>
>
>