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

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 issigtrapicmisscheck() {  471     55 |  assert(UseSIGTRAP, "precondition");  471     56 |     return MacroAssembler::istrapicmisscheck(longat(0)); ===========================^ "/priv/d031900/openjdk/jdk-hs/source/src/hotspot/cpu/ppc/nativeInstppc.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 CPUHEADER(interpreterRT) ... }; which pulls in the content of interpreterRTppc.hpp. interpreterRTppc.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 osxxxxxx "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 interpreterRTppc.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.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 nativeInstppc.hpp or does just removing it from interpreterRTppc.hpp fix the problem? In this case it seems to be sufficient to remove the include from interpreterRTppc.hpp: - .../hotspot $ hg diff diff -r d3daa45c2c8d src/hotspot/cpu/ppc/interpreterRTppc.hpp --- a/src/hotspot/cpu/ppc/interpreterRTppc.hpp Wed Mar 21 12:25:06 2018 +0100 +++ b/src/hotspot/cpu/ppc/interpreterRTppc.hpp Wed Mar 21 16:27:02 2018 +0100 @@ -26,7 +26,6 @@  #ifndef CPUPPCVMINTERPRETERRTPPCHPP  #define CPUPPCVMINTERPRETERRTPPCHPP -#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/interpreterRTaarch64.hpp src/hotspot/cpu/sparc/interpreterRTsparc.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/interpreterRTs390.cpp --- a/src/hotspot/cpu/s390/interpreterRTs390.cpp  Wed Mar 21 08:37:04 2018 +0100 +++ b/src/hotspot/cpu/s390/interpreterRTs390.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);  fpargnr = 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 interpreterframestackdirection() to target specific hpp files (even though they're all -1), pdlastframe to thread.cpp because there isn't a thread.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