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

[David Holmes]

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