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

[Thomas Stüfe]

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.

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