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