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

[Erik Joelsson]

Hello Kim,

On 2019-04-03 16:34, Kim Barrett wrote:
>> On Apr 3, 2019, at 9:51 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
>> On 2019-04-02 16:02, Kim Barrett wrote:
>>>> 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. […]
>> 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.
> I disagree that it's mostly a problem on our internal Mach5 builds.
> There are paths in my development repos that exceed the clipped size
> in the examples in that bug, and I don't think I have a very deep
> hierarchy. Using a workspace-relative path in exception messages might
> be enough to still address JDK-8204551 though.
Right, even more reason to find a solution then.
>> 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?
> Given that our build system is mostly unhappy with basename collisions
> (other than open/custom overrides), I don't think there's much of a
> usage problem with using a basename-like approach everywhere either.

It's also allowed, though perhaps not used, to override less specific 
files with more specific, as in OS specific file foo.cpp would override 
shared foo.cpp.

I won't update the other references in this patch. I will see if anyone 
else thinks it's a good enough idea to give it a go. That kind of change 
would not affect the build, nor the full paths in binaries, just 
(hopefully) improve readability of error/log messages.

> While researching what various platforms do in this area, I ran across
> a suggestion that debug builds should maybe use full paths and release
> builds use shortened paths.  That seems like an interesting idea, though
> we still want short paths in the exceptions.hpp case.
If you really think this is worth it, I could implement it, but I would 
rather keep it the same if possible.
> I tend to expect (modern) compilers to be pretty good at obvious
> optimizations, though admit I'm occasionally sadly disappointed.
>
>>> 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.
> I agree that either approach does that.  I looked at the webrev and
> it's a bunch of build system changes that I don't feel qualified to
> review, so I suggested an alternative that I understand.  We each have
> our preferred tools.  I don't object to the build-system-based
> approach, just can't give an actual thumbs-up.

Right, and I appreciate the input! But could you at least review the 
hotspot part, so I can get someone else to review the build part?

/Erik