Hello Kim,
On 2019-04-02 16:02, Kim Barrett wrote:
On Apr 2, 2019, at 5:39 PM, Erik Joelsson <erik.joels...@oracle.com> wrote:
In JDK-8204551, exceptions.hpp started using THIS_FILE instead of __FILE__ to
generate exception messages. This is causing the precompiled header to no
longer provide any benefit on Linux/GCC. The problem is that the THIS_FILE
macro is provided on the command line and is different for each compile unit.
Because of this, it seems GCC invalidates the precompiled header completely.
(On other platforms, the value of THIS_FILE is instead ignored).
The goal of JDK-8204551 was to shorten the path to the file where an exception
was raised to avoid truncation. This patch provides a different approach to
achieve this, that also fixes the issue for other platforms and compilers. It
also fixes the performance issue with precompiled headers.
For Hotspot (at least for now), we stop setting -DTHIS_FILE on the compiler
command line completely. Instead this macro is defined in macros.hpp, based on
__FILE__ but with an offest. This offset is calculated in configure. For newer
versions of GCC (8+) there is a new flag, -fmacro-prefix-map=, which can be
used to rewrite the __FILE__ macro. Configure checks if this flag is available
and uses it instead of the offset macro if possible.
I only touched the one usage of THIS_FILE/__FILE__ in this patch. It's possible
we should rewrite all references to __FILE__ to improve debug/error.
Bug: https://bugs.openjdk.java.net/browse/JDK-8221851
Webrev: http://cr.openjdk.java.net/~erikj/8221851/webrev.01/index.html
/Erik
Here's an alternative approach that seems simpler. Don't pass
THIS_FILE to HotSpot code (or ignore it if that works). Instead add
inline const char* this_file_helper(const char* s) {
const char* result = strrchr(s, '/');
return (result == NULL) ? s : (result + 1);
}
and in the one place where HotSpot is using THIS_FILE
(exceptions.hpp), instead use
this_file_helper(__FILE__)
(This assumes the path separator is '/', but there's probably a
constant for that somewhere if it's needed.)
(this_file_helper() is only needed to deal with the case where
__FILE__ contains no path separators; that might not even be an issue,
in which case it can be dropped and just add 1 to the strrchr result.
Also not wedded to that name; this is more or less basename(3).)
Empirically, gcc will optimize that all quite nicely. I don't know
about other platforms, but the worst that happens is we take a small
runtime hit when "throwing" an exception.
This kind of solution will also work. It's worth noting that your fix
will work like basename and just print the filename (as the current
state in hotspot does for exceptions if compiled with GCC). I think
printing the complete relative path to the workspace root is an
improvement over this, and I think the only reason it's currently
printing just the filename is that that's what appeared available as an
alternative. The issue with truncated paths is mostly happening on our
internal Mach5 builds where the workspace root is usually located in an
extremely deep path hierarchy.
Additionally, I would like to replace all (or at least most) instances
of __FILE__ with my new THIS_FILE, and I suspect some other places would
be more performance sensitive. Do you see any problem with doing that
substitution?
This doesn't eliminate the full pathname string embedded in the
executable, but I don't think the proposed FILE_MACRO_OFFSET will do
that either. Those get fixed by -fmacro-prefix-map=.
Correct. While solving this universally would be nice, it wasn't the
goal of this bug. The main goal was to get precompiled headers to work
with GCC again.
/Erik
