Hi Magnus, Erik, do I understand you correctly that you both are fine with my proposed fix and that we leave all the other enhancements/discussion for the future?
Thank you and best regards, Volker On Mon, Dec 3, 2018 at 12:27 PM Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> wrote: > > On 2018-11-30 19:03, Volker Simonis wrote: > > On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson <erik.joels...@oracle.com> > wrote: > > Hello Volker, > > The fix looks good. Thanks for finding and fixing it! > > Thanks! > > Now for some history on why THIS_FILE. The short story is that it's for > more reproducible and comparable builds. > > When we started the build infra project, one of the design decisions was > to use absolute paths everywhere to avoid having to keep track of the > current directory, and to make all command lines in the build be simply > copy and paste in a terminal to rerun. > > A consequence of this was that the __FILE__ macro then also expands to > absolute paths. This made binary build comparisons much harder. Very > often (especially in the build infra project itself) we use elaborate > comparison methods to verify that a build change does not change the > output of the build in any unwanted way. We then introduced the > THIS_FILE macro to get rid of the absolute paths baked into our binaries > which got rid of a huge source of binary noise preventing reproducible > builds. > > Note that two different builds with slightly different output > directories (or in the build-infra project case, completely different > output structure for generated sources) will generate absolute source > paths of different lengths. This will cause otherwise equivalent > binaries to differ greatly due to different alignment, not just because > of different contents in those strings. > > With this change, we could count on object files (at least for GCC) to > always end up binary equivalent. > > In my long term vision, I would like to get the OpenJDK build even more > reproducible, but it's currently not a high priority task. I would be > very hard to convince of reducing the level of reproducible output we > have though. > > Thanks for the background information. But as far as I can see, this > currently only works because "THIS_FILE" is always empty which of > course makes builds to various locations highly comparable :) On the > other hand, HotSpot is not using THIS_FILE at all and "__FILE__" quite > a lot. > > No, it's not. It will work just as well when THIS_FILE once more is fixed, > since > /tmp/foo/src/java.base/.../fooimpl.c will have -DTHIS_FILE="fooimpl.c" > just as > /home/chthulu/puny_humans_projects/jdk/src/java.base/.../fooimpl.c will have > -DTHIS_FILE="fooimpl.c" > > So the builds of fooimpl.c will be identical. Or, at least, not dependent on > His R'lyehian Highness choice of directory names. > > /Magnus > > > > Don't get me wrong. I highly appreciate the feature of having absolute > path names in the build to make all command lines in the build > self-contained (I use that feature every day :). And I also support > the goal of making builds even more reproducible. But does this goal > not apply to hotspot (or is it just on the TODO list ?). > > In the end, I'm happy with the current, minimal fix which at least > gets the logs working again. > > And maybe for the follow up change we should then better move all > "__FILE__" occurrences in HotSpot to "THIS_FILE" instead of getting > rid of "THIS_FILE"? > > Best regards, > Volker > > /Erik > > On 2018-11-30 09:05, Volker Simonis wrote: > > Hi, > > can I please have a review for the following trivial fix: > > http://cr.openjdk.java.net/~simonis/webrevs/2018/8214534/ > https://bugs.openjdk.java.net/browse/JDK-8214534 > > DISCLAIMER: "XS" refers to the size of the fix, not the size of the > explanation :) > > Currently the compilation of native files defines "THIS_FILE" to hold > the name of the current compilation unit. However, setting "THIS_FILE" > in NativeCompilation.gmk is broken and results in "THIS_FILE" always > being the empty string. I first thought that this is just a simple > quoting issue, but after I couldn't get it working, I found the > following explanation in the GNU Make manual [1]: > > "A common mistake is attempting to use $@ within the prerequisites > list; this will not work. However, there is a special feature of GNU > make, secondary expansion (see Secondary Expansion), which will allow > automatic variable values to be used in prerequisite lists." > > I'm not a Make expert, but I think this quote doesn't apply to "$@" > only, but to all automatic variables. The proposed solution (i.e. > "Secondary Expansion" [2]) seems overly complex for this problem. I > think the solution in the patch is much simpler and works "just as > well" :) > > The other question is of course why do we need "THIS_FILE" at all? It > is used for various native logs in the class library (not in HotSpot) > which use the value of "THIS_FILE" to decorate their output with the > current file name. On the other hand, we already have the standard, > predefined "__FILE__" macro which could be used instead (and indeed, > if "THIS_FILE" isn't defined, the various logging routines fall back > to using "__FILE__"). > > The only explanation I could come up for having "THIS_FILE" until now > is that "__FILE__" may contain the full path name of the compilation > unit while we only want the simple file name (without path) in the > log. However, in order to solve this "path" problem, we can use > simpler solutions. > > Some call sites (e.g. > "src/jdk.jdwp.agent/share/native/libjdwp/log_messages.h") already use > helper functions like "file_basename()" to cut off a potential path > component from "THIS_FILE". Other call sites (e.g. > "src/java.instrument/share/native/libinstrument/JPLISAssert.h" or > "src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h") currently > simply use the value of "THIS_FILE" directly. But they could be easily > fixed by either using "file_basename()" there as well or even simpler, > wrapping "__FILE__" into another macro which calls "strrchr()" to do > the same work. > > So as a follow up to this change, I'd like to propose another change > which completely removes "THIS_FILE" and changes all users of > "THIS_FILE" to use "__FILE__" instead. This would also shorten our > compile command lines (which doesn't happen too often :) What do you > think? > > Thank you and best regards, > Volker > > [1] > https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html > [2] > https://www.gnu.org/software/make/manual/html_node/Secondary-Expansion.html#Secondary-Expansion > >
