On Fri, Apr 5, 2019 at 10:51 AM David Holmes <david.hol...@oracle.com> wrote:
> 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 > Thanks. Okay, I agree. In general, it would be useful to have a variant of __FILE__ which only contains the filename. ..Thomas > > > 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. > > >>>>>> > > >>>>>> > > > > ------------------------------------------------------------------------------ > > > > >>>>>> > > >>>>>> > > >>>>>> > > >>> > > > > > >
