RFR: JDK-8221851: Use of THIS_FILE in hotspot invalidates precompiled header on Linux/GCC

[Erik Joelsson]

Hello Kim,

On 2019-04-02 16:02, Kim Barrett wrote:
>> On Apr 2, 2019, at 5:39 PM, Erik Joelsson <erik.joelsson at 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

>