Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
Cool, thanks! I'll push then... On Mon, Dec 3, 2018 at 2:37 PM Magnus Ihse Bursie 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 > wrote: > > On 2018-11-30 19:03, Volker Simonis wrote: > > On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson > 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
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
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 wrote: On 2018-11-30 19:03, Volker Simonis wrote: On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson 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 alread
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
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 wrote: > > On 2018-11-30 19:03, Volker Simonis wrote: > > On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson > 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,
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
On 2018-11-30 19:03, Volker Simonis wrote: On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson 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 c
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
Hello, On 2018-11-30 10:03, Volker Simonis wrote: 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. 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"? When converting the hotspot makefiles, that was on our list, but we never got around to it. The problem with the THIS_FILE approach is that it only sets the .c/.c++ file name while many of the __FILE__ in hotspot are in header files. This means that the accompanying __LINE__ will be incorrect. At the time I didn't feel comfortable introducing such a degradation in the usefulness of Hotspot debug messages. We also have a much harder time with reproducible output in Hotspot in general, probably because it's a much bigger library and also because of C++ vs C. As it stands now, I couldn't say we have concrete plans on changing Hotspot to THIS_FILE other than a vague vision of at some point making an effort towards more reproducible builds in general. A possible different approach could be to back off on the absolute paths in the actual compile command lines by actively rewriting all paths relative to the source root before calling the compiler, and always call the compiler from there. This would preserve the absolute paths everywhere in make, but would make the cmdline files a bit less useful (though we could still make a copy-paste of the command in there behave with "(cd srcroot && compile)" construct.) This may be a better long term strategy for truly reproducible builds anyway. /Erik
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
On Fri, Nov 30, 2018 at 6:37 PM Erik Joelsson 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. 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_FI
Re: RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
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
RFR(XS): 8214534: Setting of THIS_FILE in the build is broken
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
