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