Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
Looks good to me as well. Tim On 03/02/18 07:29, Erik Joelsson wrote: This looks good to me. /Erik On 2018-03-02 04:45, Magnus Ihse Bursie wrote: On 2018-03-02 12:10, Magnus Ihse Bursie wrote: On 2018-03-02 03:01, David Holmes wrote: Hi Magnus, On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 Why did you rename generateJvmOffsetsMain.c to generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C program. Yes, but so are generateJvmOffsets.cpp. :-& There was no point in mixing a .cpp and .c file for this trivial build tool helper. In fact, I don't even understand why they are two separate files -- if I get the blessings from someone in hotspot, I'll gladly just concatenate them into a single file. Come to think about it, I don't care about the hotspot group's blessing. ;-) I just moved the main function into the generateJvmOffsets.cpp file. It was just silly having it as a separate file. ! # Since we cannot generated JvmOffsets.cpp as part of the gensrc step, Comment doesn't read right. Typo, should be "generate". I'll fix. Updated. I also restored the extra ( ) in ExecuteWithLog with redirection, and added an additional ( ) for one case that was previously missing one. Finally I also added the changes to dtrace that Erik requested for JDK-8198859, but which was already pushed by that time. New webrev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02 /Magnus
Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
This looks good to me. /Erik On 2018-03-02 04:45, Magnus Ihse Bursie wrote: On 2018-03-02 12:10, Magnus Ihse Bursie wrote: On 2018-03-02 03:01, David Holmes wrote: Hi Magnus, On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 Why did you rename generateJvmOffsetsMain.c to generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C program. Yes, but so are generateJvmOffsets.cpp. :-& There was no point in mixing a .cpp and .c file for this trivial build tool helper. In fact, I don't even understand why they are two separate files -- if I get the blessings from someone in hotspot, I'll gladly just concatenate them into a single file. Come to think about it, I don't care about the hotspot group's blessing. ;-) I just moved the main function into the generateJvmOffsets.cpp file. It was just silly having it as a separate file. ! # Since we cannot generated JvmOffsets.cpp as part of the gensrc step, Comment doesn't read right. Typo, should be "generate". I'll fix. Updated. I also restored the extra ( ) in ExecuteWithLog with redirection, and added an additional ( ) for one case that was previously missing one. Finally I also added the changes to dtrace that Erik requested for JDK-8198859, but which was already pushed by that time. New webrev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02 /Magnus
Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
On 2018-03-02 12:10, Magnus Ihse Bursie wrote: On 2018-03-02 03:01, David Holmes wrote: Hi Magnus, On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 Why did you rename generateJvmOffsetsMain.c to generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C program. Yes, but so are generateJvmOffsets.cpp. :-& There was no point in mixing a .cpp and .c file for this trivial build tool helper. In fact, I don't even understand why they are two separate files -- if I get the blessings from someone in hotspot, I'll gladly just concatenate them into a single file. Come to think about it, I don't care about the hotspot group's blessing. ;-) I just moved the main function into the generateJvmOffsets.cpp file. It was just silly having it as a separate file. ! # Since we cannot generated JvmOffsets.cpp as part of the gensrc step, Comment doesn't read right. Typo, should be "generate". I'll fix. Updated. I also restored the extra ( ) in ExecuteWithLog with redirection, and added an additional ( ) for one case that was previously missing one. Finally I also added the changes to dtrace that Erik requested for JDK-8198859, but which was already pushed by that time. New webrev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02 /Magnus
Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
On 2018-03-02 03:01, David Holmes wrote: Hi Magnus, On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 Why did you rename generateJvmOffsetsMain.c to generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C program. Yes, but so are generateJvmOffsets.cpp. :-& There was no point in mixing a .cpp and .c file for this trivial build tool helper. In fact, I don't even understand why they are two separate files -- if I get the blessings from someone in hotspot, I'll gladly just concatenate them into a single file. I agree the logic is quite confusing. I think this build logic was victim of the CPP_FLAGS (meaning C preprocessor) to CXX_FLAGS (meaning C++ flags) renaming. But this is a trivial C program and should require trivial C compiler flags. I don't see it should be being built with all the JVM_CFLAGS. The latter may be harmless but it seems wrong to lump this in together with other things. Actually, no. Or, maybe it was a victim of CPP_FLAGS/CXX_FLAGS confusion, too. But the JVM_CFLAGS *are* needed. Otherwise I'd removed them, trust me. And I would have moved the entire piece of code to gensrc, where it belongs. (This is just about compiling a build tool that will generate source code that should be later compiled, typical gensrc stuff). But, this file includes a lot of hotspot include files. And for that to work, we need the -I and -D flags from JVM_CFLAGS. We probably don't need any other parts of the JVM_CFLAGS, so in theory, we could probably split JVM_CFLAGS into a "defines and include paths" part, and a "rest" part. But I would not bet on it, suddenly you'd have some kind of option (-xc99?) that modifies the parsing of the include files... This is the general problem with all dtrace stuff, it needs to poke it's fingers deep down in the libjvm. :( make/hotspot/lib/CompileDtracePreJvm.gmk ! # Since we cannot generated JvmOffsets.cpp as part of the gensrc step, Comment doesn't read right. Typo, should be "generate". I'll fix. /Magnus Thanks, David /Magnus
Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
Hi Magnus, On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 Why did you rename generateJvmOffsetsMain.c to generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C program. I agree the logic is quite confusing. I think this build logic was victim of the CPP_FLAGS (meaning C preprocessor) to CXX_FLAGS (meaning C++ flags) renaming. But this is a trivial C program and should require trivial C compiler flags. I don't see it should be being built with all the JVM_CFLAGS. The latter may be harmless but it seems wrong to lump this in together with other things. make/hotspot/lib/CompileDtracePreJvm.gmk ! # Since we cannot generated JvmOffsets.cpp as part of the gensrc step, Comment doesn't read right. Thanks, David /Magnus
Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
Hello, I don't think you can remove the extra ( ) around the preprocessor commands. I added those to avoid race conditions in JDK-8158629. My conclusion then was that any command that redirected stdout needed to be wrapped in (). Otherwise this looks ok. /Erik On 2018-02-28 16:48, Magnus Ihse Bursie wrote: We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 /Magnus
RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace
We're doing a lot of weird compilation stuff for dtrace. With this patch, most of the weirdness is removed. The remaining calls to $(CC) -E has been changed to $(CPP) to clarify that we do not compile, we just use the precompiler. One of the changes I made was to actually split up the last and final dtrace call into a separate preprocessing step. However, this uses the solaris studio preprocessor instead of the ancient system preprocessor, which has changed behavior. A string like (&``_var) is now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I have worked around this by adding the preprocessed output, without the spaces, in two places. If anyone wants to dig deeper into dtrace script file syntax, or C preprocessor magic, to avoid this, let me know... (I'll just state that the "obvious" solution of sending -Xs to the preprocessor to get old-style behavior does not work: this just makes the solaris studio preprocessor call the ancient preprocessor in turn, and we've gained nothing...) Bug: https://bugs.openjdk.java.net/browse/JDK-8198862 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 /Magnus
