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

Wed Mar 21 16:24:20 UTC 2018

On Wed, Mar 21, 2018 at 5:02 PM, <coleen.phillimore at oracle.com> wrote:

>
>
> 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> 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_mis
>> s_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?
>

you were not lying, this was tedious :-)

This is a good cleanup! Love that so much unnecessary inline functions are
moved to cpp files.

Remarks:

src/hotspot/cpu/aarch64/interpreterRT_aarch64.hpp
src/hotspot/cpu/sparc/interpreterRT_sparc.hpp

also include headers and should not.

--

http://cr.openjdk.java.net/~coleenp/8199809.01/webrev/src/hotspot/share/runtime/vframe.hpp.udiff.html

The prototypes of those methods moved to vframe.inline.hpp should be marked
as inline.

Thanks, Thomas

> Thanks,
> Coleen
>
> thanks,
>> Coleen
>>
>>
>>
>>
>> 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::SignatureHan
>>> dlerGenerator(
>>> +InterpreterRuntime::SignatureHandlerGenerator::SignatureHan
>>> dlerGenerator(
>>>      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
>>>>
>>>
>>>
>>
>>
>
>