RFR: JDK-8221851: Use of THIS_FILE in hotspot invalidates precompiled header on Linux/GCC

David Holmes david.holmes at oracle.com
Fri Apr 5 08:51:48 UTC 2019


Hi Thomas,

On 5/04/2019 6:23 pm, Thomas Stüfe wrote:
> Hi,
> 
> sorry, just sniping in from the side.
> 
> About the motivation: the original intent of Yasumasas patch was to 
> reduce the length of the event log messages.
> 
> I have a patch out there for RFR, since ~2 weeks or so, which largely 
> solves this problem:
> 
> https://bugs.openjdk.java.net/browse/JDK-8220762
> 
> The patch abolishes fixed-length string records in the event log in 
> favor of variable length strings, and therefore uses the event log 
> buffer much more efficiently. Truncation could still happen but is very 
> unlikely.
> 
> The patch is out since some time and has no reviews yet (Yasumasa agreed 
> to review), I did not press it since we are all very busy. But, it would 
> solve this problem, and maybe we do not need this fix at all.

It wouldn't solve the current problem - which is the invalidation of the 
precompiled header file. But after it is fixed we could regress 
Yasumasa's change which would fix the current problem.

Cheers,
David

> Unless we want a generic solution to problems like this.
> 
> Cheers, Thomas
> 
> 
> 
> 
> 
> On Fri, Apr 5, 2019 at 8:00 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     On 5/04/2019 3:23 pm, Ioi Lam wrote:
>      >
>      >
>      > On 4/4/19 10:08 PM, David Holmes wrote:
>      >> Adding back build-dev
>      >>
>      >> On 5/04/2019 2:41 pm, Ioi Lam wrote:
>      >>> #define THIS_FILE (__FILE__ + FILE_MACRO_OFFSET)
>      >>>
>      >>> This make me a little uneasy, as it could potential point past the
>      >>> end of the string.
>      >>
>      >> The intent of course is that that is impossible:
>      >> - _FILE_ is an absolute file path within the repo:
>     /something/repo/file
>      >> - FILE_MACRO_OFFSET gets you from / to the top-level repo directory
>      >> leaving "file"
>      >>
>      >> so it has to be in bounds.
>      >>
>      >>> For safety, Is it possible to get some sort of assert to make sure
>      >>> that __FILE__ always has more than FILE_MACRO_OFFSET characters?
>      >>>
>      >>> Maybe we can add a static object in macros.hpp with a constructor
>      >>> that puts __FILE__ into a list, and then we can walk the list when
>      >>> the VM starts up? E.g.
>      >>>
>      >>>      ...
>      >>>
>      >>>      #ifdef ASSERT
>      >>>      static ThisFileChecker __this_file_checker(__FILE__);
>      >>>      #endif
>      >>>
>      >>>      #endif // SHARE_UTILITIES_MACROS_HPP
>      >>
>      >> So basically you're not trusting the compiler and build system
>     to be
>      >> correct here. :)
>      >
>      > I am sure the build system is correct ..... but it's complicated.
>      >
>      > BTW, we actually have generated sources that can live outside of the
>      > source repo. And with luck, their names can actually be shorter than
>      > FILE_MACRO_OFFSET.
> 
>     Excellent point! repo sources and generated sources need not be in the
>     same tree, so you cannot use one "offset" to handle them both.
> 
>     David
>     -----
> 
> 
>      > $ cd /tmp/mybld
>      > $ find . -name \*.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_expand.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_pipeline.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_format.cpp
>      > ./hotspot/variant-server/support/adlc/dfa_x86.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_misc.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_gen.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_peephole.cpp
>      > ./hotspot/variant-server/support/adlc/ad_x86_clone.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_expand.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_pipeline.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_format.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/dfa_x86.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_misc.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_gen.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_peephole.cpp
>      > ./hotspot/variant-server/gensrc/adfiles/ad_x86_clone.cpp
>      > ./hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnter.cpp
>      >
>     ./hotspot/variant-server/gensrc/jvmtifiles/bytecodeInterpreterWithChecks.cpp
> 
>      >
>      > ./hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnterTrace.cpp
>      >
>      >>
>      >> Would it suffice to put a static assert in a common header, like
>      >> macros.hpp, that verifies the length of _FILE_ or is that not
>      >> available statically?
>      >>
>      > I don't know how a static assert would work. That's why I
>     suggested the
>      > static object with a constructor.
>      >
>      > Thanks
>      > - Ioi
>      >
>      >> Cheers,
>      >> David
>      >>
>      >>>
>      >>> Thanks
>      >>> - Ioi
>      >>>
>      >>>
>      >>> On 4/4/19 9:04 PM, David Holmes wrote:
>      >>>> Hi Erik,
>      >>>>
>      >>>> Build and hotspot changes seem okay.
>      >>>>
>      >>>> Thanks,
>      >>>> David
>      >>>>
>      >>>> On 5/04/2019 8:03 am, Erik Joelsson wrote:
>      >>>>>
>      >>>>> On 2019-04-04 14:26, Kim Barrett wrote:
>      >>>>>>
>      >>>>>> OK, I can do that.
>      >>>>>>
>      >>>>>>
>     ------------------------------------------------------------------------------
> 
>      >>>>>>
>      >>>>>> src/hotspot/share/utilities/macros.hpp
>      >>>>>>   645 #if FILE_MACRO_OFFSET
>      >>>>>>   646 #define THIS_FILE (__FILE__ + FILE_MACRO_OFFSET)
>      >>>>>>   647 #else
>      >>>>>>   648 #define THIS_FILE __FILE__
>      >>>>>>   649 #endif
>      >>>>>>
>      >>>>>> Is the "#if FILE_MACRO_OFFSET" an intentional test for 0, or
>     is this
>      >>>>>> an implicit test for "defined"?
>      >>>>>>
>      >>>>>> If the former, e.g. we're assuming it will always be defined
>     but
>      >>>>>> might
>      >>>>>> have a 0 value, then I'd skip it and just unconditionally define
>      >>>>>> THIS_FILE as (__FILE__ + FILE_MACRO_OFFSET).
>      >>>>>
>      >>>>> Right, that makes sense. I was sort of hedging for all
>      >>>>> possibilities here, but as the build logic is currently
>     structured,
>      >>>>> it will always be defined, just sometimes 0.
>      >>>>>
>      >>>>> New webrev: http://cr.openjdk.java.net/~erikj/8221851/webrev.02/
>      >>>>>
>      >>>>> /Erik
>      >>>>>
>      >>>>>> If the latter, some compilers will (with some warning levels or
>      >>>>>> options, such as gcc -Wundef) complain about the (specified
>     by the
>      >>>>>> standard) implicit conversion to 0 for an unrecognized
>     identifier in
>      >>>>>> an #if expression, and an #ifdef should be used to protect
>     against
>      >>>>>> that.
>      >>>>>>
>      >>>>>>
>     ------------------------------------------------------------------------------
> 
>      >>>>>>
>      >>>>>>
>      >>>>>>
>      >>>
>      >
> 


More information about the build-dev mailing list