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