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.hol...@oracle.com
<mailto:david.hol...@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.
>>>>>>
>>>>>>
------------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>>
>>>
>
