Hello Kim,
On 2019-04-03 16:34, Kim Barrett wrote:
On Apr 3, 2019, at 9:51 AM, Erik Joelsson <erik.joels...@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
