RFR(XS): 8214534: Setting of THIS_FILE in the build is broken

Mon Dec 3 13:39:48 UTC 2018

Cool, thanks!
I'll push then...
On Mon, Dec 3, 2018 at 2:37 PM Magnus Ihse Bursie
<magnus.ihse.bursie at oracle.com> wrote:
>
> On 2018-12-03 14:31, Volker Simonis wrote:
>
> 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?
>
> Yes, sorry I was not clear. Your fix looks good to me.
>
> We can save the rest of the discussion of reproducible builds for a rainy day.
>
> /Magnus
>
> Thank you and best regards,
> Volker
>
> On Mon, Dec 3, 2018 at 12:27 PM Magnus Ihse Bursie
> <magnus.ihse.bursie at oracle.com> wrote:
>
> On 2018-11-30 19:03, Volker Simonis wrote:
>
> On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson <erik.joelsson at 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
>
>
>