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

[Erik Joelsson]

Hello Volker,

The fix looks good. Thanks for finding and fixing it!

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.

/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