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

Mon Dec 3 11:25:37 UTC 2018

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