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