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 08:25:57 UTC 2019


p.s.: Review is at hs-runtime-dev:
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-March/033150.html
.

On Fri, Apr 5, 2019 at 10:23 AM Thomas Stüfe <thomas.stuefe at gmail.com>
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.
>
> 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.
>> >>>>>>
>> >>>>>>
>> ------------------------------------------------------------------------------
>>
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>
>> >
>>
>


More information about the build-dev mailing list