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 18:21:07 UTC 2018
Thank you for reviewing this.
On 3/21/18 12:24 PM, Thomas Stüfe wrote:
>
>
> On Wed, Mar 21, 2018 at 5:02 PM, <coleen.phillimore at oracle.com
> <mailto: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
>> <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?
>
>
> 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.
Yes, I removed these headers when I added the comment above and rebuilt
the sparc version.
>
> --
>
> http://cr.openjdk.java.net/~coleenp/8199809.01/webrev/src/hotspot/share/runtime/vframe.hpp.udiff.html
> <http://cr.openjdk.java.net/%7Ecoleenp/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.
Okay, I'll change it and retest.
Thanks,
Coleen
>
> 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 <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