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

Thomas Stüfe thomas.stuefe at gmail.com
Fri Apr 5 09:18:16 UTC 2019


On Fri, Apr 5, 2019 at 10:51 AM David Holmes <david.holmes at oracle.com>
wrote:

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

Thanks. Okay, I agree.

In general, it would be useful to have a variant of __FILE__ which only
contains the filename.

..Thomas


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