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

Mon Dec 3 13:35:06 UTC 2018

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
>>
>>