Re: RFR: 8296886: Fix various include sort order issues [v2]
On Wed, 16 Nov 2022 16:17:48 GMT, Stefan Karlsson wrote: >> The sorted blocks of includes have deteriorated to the point that I felt >> compelled to clean up some of the issues. >> >> *EDIT*: The below discussion has been deferred out of this PR. Now this only >> deals with fixing the placement and sorting of includes, plus some >> surrounding blank lines. >> >> One of the more prevalent issues is that files in src/hotspot/share/include >> are not properly sorted. There has been some discussion that that was done >> on purpose, but it just adds another exception to the include rules that >> don't have any practical purposes, IMHO. It also goes against our written >> style guide around include files. One argument why it was OK have the files >> in include/ pushed up to the top of the sorted block, was that the file was >> included without specifying a directory. That's an argument that contradicts >> how we treat platform-dependent files, which (unfortunately) often also are >> specified without a prefixed directory, so I don't think that's a good >> enough argument, again IMHO. To remove this special case, I've removed the >> extraneous make file entry to have src/hotspot/share/include in the set of >> directories to search for headers when compiling HotSpot. Now all the header >> files in src/hotspot/share/include gets included by specifying the path from >> src/hotspot/share , just like the other platform-independent headers in HotSpot. >> >> While going over the include headers I've also cleaned up surrounding >> whitespaces and incorrect include guards. > > Stefan Karlsson has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Cleanups > - Merge remote-tracking branch 'upstream/master' into > various_include_order_fixes > - Various include order fixes Looks good and is an improvement. About the removed comments after includes, I am sometimes guilty of this to mark includes that came just for one specific little thing - the implied hope is that disentangling includes and removing unnecessary ones periodically is easier that way. However, this never worked. I wish we had an automatic include "GC" process to reduce the includes, like Ioi sometimes does manually. - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.org/jdk/pull/11108
Re: RFR: 8296886: Fix various include sort order issues [v2]
On Wed, 23 Nov 2022 07:12:13 GMT, Kim Barrett wrote: > > I'm not sure why conditional includes (that don't rely on macros.hpp) need > > to come at the end rather than in normal sort order? I don't care either > > way but a rationale for this would be good if it is to be the preferred > > style. > > Because the Style Guide says: > > * Put conditional inclusions (`#if ...`) at the end of the include list. Ah I see. Thanks for that @kimbarrett > > I think most of our conditional includes these days are to support > conditional features. It makes sense to group all the additional includes > related to a feature. True. There are a lot of single includes but it makes sense to have one simple rule. - PR: https://git.openjdk.org/jdk/pull/11108
Re: RFR: 8296886: Fix various include sort order issues [v2]
On Wed, 16 Nov 2022 16:17:48 GMT, Stefan Karlsson wrote: >> The sorted blocks of includes have deteriorated to the point that I felt >> compelled to clean up some of the issues. >> >> *EDIT*: The below discussion has been deferred out of this PR. Now this only >> deals with fixing the placement and sorting of includes, plus some >> surrounding blank lines. >> >> One of the more prevalent issues is that files in src/hotspot/share/include >> are not properly sorted. There has been some discussion that that was done >> on purpose, but it just adds another exception to the include rules that >> don't have any practical purposes, IMHO. It also goes against our written >> style guide around include files. One argument why it was OK have the files >> in include/ pushed up to the top of the sorted block, was that the file was >> included without specifying a directory. That's an argument that contradicts >> how we treat platform-dependent files, which (unfortunately) often also are >> specified without a prefixed directory, so I don't think that's a good >> enough argument, again IMHO. To remove this special case, I've removed the >> extraneous make file entry to have src/hotspot/share/include in the set of >> directories to search for headers when compiling HotSpot. Now all the header >> files in src/hotspot/share/include gets included by specifying the path from >> src/hotspot/share , just like the other platform-independent headers in HotSpot. >> >> While going over the include headers I've also cleaned up surrounding >> whitespaces and incorrect include guards. > > Stefan Karlsson has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Cleanups > - Merge remote-tracking branch 'upstream/master' into > various_include_order_fixes > - Various include order fixes Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.org/jdk/pull/11108
Integrated: 8297350: Update JMH devkit to 1.36
On Mon, 21 Nov 2022 15:36:41 GMT, Aleksey Shipilev wrote: > JMH 1.36 was released, so we can bump the devkit too. > > Additional testing: > - [x] Ad-hoc benchmarks with newly generated devkit This pull request has now been integrated. Changeset: f26bd4e0 Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/f26bd4e0e8b68de297a9ff93526cd7fac8668320 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8297350: Update JMH devkit to 1.36 Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/11271
Re: RFR: 8296886: Fix various include sort order issues [v2]
On Wed, 23 Nov 2022 04:47:11 GMT, David Holmes wrote: > I'm not sure why conditional includes (that don't rely on macros.hpp) need to > come at the end rather than in normal sort order? I don't care either way but > a rationale for this would be good if it is to be the preferred style. Because the Style Guide says: * Put conditional inclusions (`#if ...`) at the end of the include list. I think most of our conditional includes these days are to support conditional features. It makes sense to group all the additional includes related to a feature. - PR: https://git.openjdk.org/jdk/pull/11108
Integrated: 8296904: Improve handling of macos xcode toolchain
On Fri, 11 Nov 2022 22:22:51 GMT, Christoph Langer wrote: > With this PR I'd like to make it easier to use dedicated installations of > Xcode on Mac OS for OpenJDK builds without having to switch the active Xcode > globally via `xcode-select`. > > I propose adding a new configure flag `--with-xcode-path` that takes the path > to an Xcode installation. If the option is set, this path is expanded to a > valid `TOOLCHAIN_PATH`. > > Furthermore, I fix detection of xcodebuild and correctly setting the sysroot > from the toolchain by moving `AC_SUBST(TOOLCHAIN_PATH)` before calling > `BASIC_SETUP_XCODE_SYSROOT` and honoring `TOOLCHAIN_PATH` in > `BASIC_SETUP_XCODE_SYSROOT` in the case when no devkit is specified. As I see > it, this is a viable fix, even if not introducing `--with-xcode-path`. This pull request has now been integrated. Changeset: 470f3424 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/470f3424fcce0e41b75cccdd5e3a56771cd07ff5 Stats: 85 lines in 3 files changed: 31 ins; 5 del; 49 mod 8296904: Improve handling of macos xcode toolchain Reviewed-by: erikj, ihse - PR: https://git.openjdk.org/jdk/pull/3
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v8]
On Tue, 22 Nov 2022 14:10:44 GMT, Magnus Ihse Bursie wrote: >> @magicus Thanks for fixing my enumeration. I recognized that you have >> regenerated all the html files with a newer pandoc and I had to resolve with >> master. I guess you'll need to regenerate with my current branch once more - >> otherwise I think I'm breaking some of the new style. > > @RealCLanger Finally regenerated once more: > [fcd20c73ed9f0a44c0614a54580c755a16387a6a](https://github.com/magicus/jdk/commit/fcd20c73ed9f0a44c0614a54580c755a16387a6a) Thanks @magicus for your help. - PR: https://git.openjdk.org/jdk/pull/3
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 14 Nov 2022 12:20:54 GMT, Julian Waters wrote: >> Sorry my eyes must be playing tricks on me. ?? >> >> Why did you need to add this here? > > It's to avoid redefining the linkage as static in os_windows.cpp (where it's > implemented) after an extern declaration (inside the class), which is > forbidden by C++11: > >> The linkages implied by successive declarations for a given entity shall >> agree. That is, within a given scope, each declaration declaring the same >> variable name or the same overloading of a function name shall imply the >> same linkage. > > While 2019 by default seems to ignore this rule and accepts the conflicting > linkage as a language extension, this can cause issues with newer and > stricter versions of the Visual C++ compiler (especially with -permissive- > passed during compilation, which Magnus and Daniel have pointed out in > another discussion will become the default mode of compilation in the > future). It's not possible to declare a static friend inside a class, so the > addition above takes advantage of another C++ feature instead: > >> ยง11.3/4 [class.friend] > A function first declared in a friend declaration has external linkage (3.5). > Otherwise, the function retains its previous linkage (7.1.1). I think the problem here is the friend declaration, which doesn't look like it's needed and could be deleted. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 21 Nov 2022 02:43:12 GMT, Julian Waters wrote: > Out of curiosity, is there a way to get the discussion on approving the use > of alignas back up? [...] A PR to address JDK-8252584 would be welcomed by me. Just do the process for Style Guide changes (see the Style Guide or previous PRs for such). I don't expect it would be very controversial. I think the only reason it hasn't already happened is because nobody has gotten around to it, or felt the need for it. JDK-8250269 touches a bit more code (mostly in stubGenerator_x86_64 and macroAssembler_x86_32), but also seems like it should be straightforward. > > The various MSVC-conditional direct uses of __declspec(align(N)) should > > probably currently be using ATTRIBUTE_ALIGNED. > > The instances of `__declspec(align())` changed here are in the native > libraries written in C, not within HotSpot itself. From what I can see at > least HotSpot never uses compiler alignment attributes directly and always > strictly sticks to `ATTRIBUTE_ALIGNED` (which is probably a good thing) You are right that the Windows-conditionalized uses are in non-HotSpot code. I missed that context when skimming through the changes. Since Visual Studio is always C++ (even though the shared files are written as C), using alignas with appropriate conditionalization in those files should be fine. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v7]
On Tue, 22 Nov 2022 19:21:51 GMT, Matias Saavedra Silva wrote: >> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, >> this flag doesn't work across JDK 19 and 20. >> >> Expected: JDK 20 should recreate the specified CDS archive >> Actual: JDK 20 cannot recognize the archive file and gives up >> >> The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified >> with tier 1-4 tests. > > Matias Saavedra Silva has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge branch 'master' into autoCreateSharedArchive_8296754 > - Added newline to end of test > - Removed unused import > - Test now passes > - Changed whitespace and comment > - Adjusted whitespace > - Added test > - 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 Approved latest changes again. Matias had to do second force-push which reverted the effects of the first force-push. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/11148
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:32:51 GMT, Magnus Ihse Bursie wrote:
>> make/langtools/tools/javacserver/client/Client.java line 148:
>>
>>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>>> 147: // javacserver.server.Server is the server main class
>>> 148: cmd.add("javacserver.server.Server");
>>
>> Did you consider referencing the class name using `Server.class.getName()`?
>
> No, can't say I have. Do you think there is anything to gain by doing that?
> (Apart from possibly getting this right automatically if the class is renamed
> by IDE tools) (I'm not sure it would be a win in readability)
I like to reference class names that way so that it's typed instead of just a
string. If the class changes name, you get a compilation error, and the IDE
would handle refactoring automatically. Not a strong need in this particular
instance, just something I generally tend to do.
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297455: Use the official ToolProvider API to call javac
On Tue, 22 Nov 2022 20:50:04 GMT, Magnus Ihse Bursie wrote: > In JDK-8297041, the javacserver was moved out of the JDK itself, and lifted > out as a separate buildtool. Due to this, internal classes in jdk.compiler > were no longer available. Therefore, the closest way to calling javac as > before were to use `com.sun.tools.javac.Main.compile()`. This method is > however deprecated for removal, and relying on that was only needed as a > temporary measure during the transition. > > After the major refactoring of the javacserver tool in JDK-8297444, it will > be easy to replace the Main.compile API with the official ToolProvider API > instead. Well, I assume so. If this does not work properly, the problem will most likely be already in JDK-8297444; this patch is just choosing a different path to end up in the same class, as far as I can understand. The thing that makes (or should make) us chose the interim javac is the `$(INTERIM_LANGTOOLS_ARGS)` which is passed to java when launching the server. (This is since these flags are passed in the config file to the client as `javacmd`). I'll add some test output code to the interim compiler to verify that it is actually executed, both in JDK-8297444 and in this PR. - PR: https://git.openjdk.org/jdk/pull/11299
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:15:20 GMT, Erik Joelsson wrote:
> Things like incremental builds, some forced failure conditions etc?
I tested simple incremental builds but I should probably make a bit more
advanced versions where I modify different files to trigger more real-life like
scenarios, including compilation errors. Good point.
> make/langtools/tools/javacserver/client/Client.java line 148:
>
>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>> 147: // javacserver.server.Server is the server main class
>> 148: cmd.add("javacserver.server.Server");
>
> Did you consider referencing the class name using `Server.class.getName()`?
No, can't say I have. Do you think there is anything to gain by doing that?
(Apart from possibly getting this right automatically if the class is renamed
by IDE tools) (I'm not sure it would be a win in readability)
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
> Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Fix typo #2 Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Fix typo Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/11298/files - new: https://git.openjdk.org/jdk/pull/11298/files/3dd8af2a..2a0e0975 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11298=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11298=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297455: Use the official ToolProvider API to call javac
On Tue, 22 Nov 2022 20:50:04 GMT, Magnus Ihse Bursie wrote: > In JDK-8297041, the javacserver was moved out of the JDK itself, and lifted > out as a separate buildtool. Due to this, internal classes in jdk.compiler > were no longer available. Therefore, the closest way to calling javac as > before were to use `com.sun.tools.javac.Main.compile()`. This method is > however deprecated for removal, and relying on that was only needed as a > temporary measure during the transition. > > After the major refactoring of the javacserver tool in JDK-8297444, it will > be easy to replace the Main.compile API with the official ToolProvider API > instead. Marked as reviewed by erikj (Reviewer). Just to make sure, will this find the interim javac before the bootjdk javac? How is this enforced? - PR: https://git.openjdk.org/jdk/pull/11299
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote:
> Now that the javacserver no longer has any ambitions outside being a
> buildtool customized for the JDK build process, a lot of abstractions and
> generalizations can be removed.
>
> This will allow the actual behavior to be more clearly visible, and will help
> debugging the issues we are still seeing (most likely race conditions), and
> to convert the tool to use the ToolProvider API in the future.
Only some minor comments. Looks good to me overall. The proof is in the
building.
Have you tried more than just full builds on all platforms? Things like
incremental builds, some forced failure conditions etc?
make/langtools/tools/javacserver/client/Client.java line 140:
> 138:
> 139: /*
> 140: * Fork a server process process and wait for server to come around
Suggestion:
* Fork a server process and wait for server to come around
make/langtools/tools/javacserver/client/Client.java line 148:
> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
> 147: // javacserver.server.Server is the server main class
> 148: cmd.add("javacserver.server.Server");
Did you consider referencing the class name using `Server.class.getName()`?
make/langtools/tools/javacserver/server/Server.java line 200:
> 198:
> 199: // Set up logging for this thread. Stream back logging
> messages to
> 200: // client on the format format "level:msg".
Suggestion:
// client on the format "level:msg".
-
Marked as reviewed by erikj (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v10]
On Tue, 22 Nov 2022 21:02:58 GMT, Christoph Langer wrote: >> With this PR I'd like to make it easier to use dedicated installations of >> Xcode on Mac OS for OpenJDK builds without having to switch the active Xcode >> globally via `xcode-select`. >> >> I propose adding a new configure flag `--with-xcode-path` that takes the >> path to an Xcode installation. If the option is set, this path is expanded >> to a valid `TOOLCHAIN_PATH`. >> >> Furthermore, I fix detection of xcodebuild and correctly setting the sysroot >> from the toolchain by moving `AC_SUBST(TOOLCHAIN_PATH)` before calling >> `BASIC_SETUP_XCODE_SYSROOT` and honoring `TOOLCHAIN_PATH` in >> `BASIC_SETUP_XCODE_SYSROOT` in the case when no devkit is specified. As I >> see it, this is a viable fix, even if not introducing `--with-xcode-path`. > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Regenerate once more Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.org/jdk/pull/3
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v10]
> With this PR I'd like to make it easier to use dedicated installations of > Xcode on Mac OS for OpenJDK builds without having to switch the active Xcode > globally via `xcode-select`. > > I propose adding a new configure flag `--with-xcode-path` that takes the path > to an Xcode installation. If the option is set, this path is expanded to a > valid `TOOLCHAIN_PATH`. > > Furthermore, I fix detection of xcodebuild and correctly setting the sysroot > from the toolchain by moving `AC_SUBST(TOOLCHAIN_PATH)` before calling > `BASIC_SETUP_XCODE_SYSROOT` and honoring `TOOLCHAIN_PATH` in > `BASIC_SETUP_XCODE_SYSROOT` in the case when no devkit is specified. As I see > it, this is a viable fix, even if not introducing `--with-xcode-path`. Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Regenerate once more - Changes: - all: https://git.openjdk.org/jdk/pull/3/files - new: https://git.openjdk.org/jdk/pull/3/files/2a5e4e44..733c300f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=3=09 - incr: https://webrevs.openjdk.org/?repo=jdk=3=08-09 Stats: 22 lines in 1 file changed: 17 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/3.diff Fetch: git fetch https://git.openjdk.org/jdk pull/3/head:pull/3 PR: https://git.openjdk.org/jdk/pull/3
RFR: 8297455: Use the official ToolProvider API to call javac
In JDK-8297041, the javacserver was moved out of the JDK itself, and lifted out as a separate buildtool. Due to this, internal classes in jdk.compiler were no longer available. Therefore, the closest way to calling javac as before were to use `com.sun.tools.javac.Main.compile()`. This method is however deprecated for removal, and relying on that was only needed as a temporary measure during the transition. After the major refactoring of the javacserver tool in JDK-8297444, it will be easy to replace the Main.compile API with the official ToolProvider API instead. - Depends on: https://git.openjdk.org/jdk/pull/11298 Commit messages: - 8297455: Use the official ToolProvider API to call javac Changes: https://git.openjdk.org/jdk/pull/11299/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11299=00 Issue: https://bugs.openjdk.org/browse/JDK-8297455 Stats: 10 lines in 1 file changed: 7 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11299.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11299/head:pull/11299 PR: https://git.openjdk.org/jdk/pull/11299
Re: RFR: 8297455: Use the official ToolProvider API to call javac
On Tue, 22 Nov 2022 20:50:04 GMT, Magnus Ihse Bursie wrote: > In JDK-8297041, the javacserver was moved out of the JDK itself, and lifted > out as a separate buildtool. Due to this, internal classes in jdk.compiler > were no longer available. Therefore, the closest way to calling javac as > before were to use `com.sun.tools.javac.Main.compile()`. This method is > however deprecated for removal, and relying on that was only needed as a > temporary measure during the transition. > > After the major refactoring of the javacserver tool in JDK-8297444, it will > be easy to replace the Main.compile API with the official ToolProvider API > instead. Note that this PR is dependent on JDK-8297444 (PR: https://github.com/openjdk/jdk/pull/11298) - PR: https://git.openjdk.org/jdk/pull/11299
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote: > Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. The remaining changes are "pure" refactorings. I have moved code around, and reworked the abstraction layers. Basically, I put everything pertaining to the client in a long sequence (calling the client is basically a trivial sequence of events), and similarly for the server. Code that were shared between both were put in the `shared` or `util` packages (the latter for more auxiliary classes, the former for more central). Everything else were mercilessly removed. I then abstracted out what seemed like reasonable parts from the long sequences in Client and Server, in the process mostly recreating the thread pool and idle detection classes of the former structure (so git will present them as renames). I've been extra careful not to change any behavior regarding the portfile locking and synchronization, or the actual protocol exchanged by the server and client (with one exception: the exit code from javac is now returned as an integer, and not as a string representing the `Result` enum.) I believe there are a number of places where races can be introduced, and I intend to come back later and try to patch this. But I deemed it essential that I do not change any behavior in that regard in this patch, otherwise it would be hopeless to know if a potential future regression was caused by a mistake in my refactoring, or that any changed behavior was faulty. - PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote: > Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. I realize this PR can be a bit of a challenge to review. I'll explain the changes I have done. The "actual" changes I have made is to how the javacserver tool is called from the makefiles, and how the client starts the server. The client now takes a `--conf=` argument, instead of `--server:conf=`. This argument has to be the first argument; the rest of the argument line is passed on to javac in the server. This allowed me to remove all the complex option processing stuff. Also, the client cannot any longer be called in "stand-alone" mode where no server is used (basically a complex version of plain `javac`). Secondly, the server is now started with a separate main method in the Server class. This means the entry point need not determine if it is being called as a client or a server, simplifying further. The server takes exactly one argument, the path of the port file. That means the last of the option processing could go. Third, the configuration file has changed slightly. Instead of a `servercmd` there is now a `javacmd`, which tells the client how to start java (suitable path to the java executable, and proper flags) when launching the server. The actual class used to launch the server is known by the client and does not have to be provided by the makefile. This change made new configuration files incompatible with old files; it turned out make had a bit of a problem of recognizing the file needed to be updated, so I chose a slightly different name for the configuration file, to avoid strange recompilation errors when this patch gets integrated. - PR: https://git.openjdk.org/jdk/pull/11298
RFR: 8297444: Refactor the javacserver build tool
Now that the javacserver no longer has any ambitions outside being a buildtool customized for the JDK build process, a lot of abstractions and generalizations can be removed. This will allow the actual behavior to be more clearly visible, and will help debugging the issues we are still seeing (most likely race conditions), and to convert the tool to use the ToolProvider API in the future. - Commit messages: - 8297444: Refactor the javacserver build tool Changes: https://git.openjdk.org/jdk/pull/11298/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11298=00 Issue: https://bugs.openjdk.org/browse/JDK-8297444 Stats: 3440 lines in 35 files changed: 1198 ins; 2212 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v7]
> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, > this flag doesn't work across JDK 19 and 20. > > Expected: JDK 20 should recreate the specified CDS archive > Actual: JDK 20 cannot recognize the archive file and gives up > > The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified > with tier 1-4 tests. Matias Saavedra Silva has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' into autoCreateSharedArchive_8296754 - Added newline to end of test - Removed unused import - Test now passes - Changed whitespace and comment - Adjusted whitespace - Added test - 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 - Changes: https://git.openjdk.org/jdk/pull/11148/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11148=06 Stats: 249 lines in 8 files changed: 191 ins; 15 del; 43 mod Patch: https://git.openjdk.org/jdk/pull/11148.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11148/head:pull/11148 PR: https://git.openjdk.org/jdk/pull/11148
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v6]
> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, > this flag doesn't work across JDK 19 and 20. > > Expected: JDK 20 should recreate the specified CDS archive > Actual: JDK 20 cannot recognize the archive file and gives up > > The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified > with tier 1-4 tests. Matias Saavedra Silva has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Added newline to end of test - Removed unused import - Test now passes - Changed whitespace and comment - Adjusted whitespace - Added test - 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 - Merge branch 'master' of https://github.com/openjdk/jdk - Merge branch 'master' of https://github.com/openjdk/jdk - Merge branch 'master' of https://github.com/openjdk/jdk - ... and 2 more: https://git.openjdk.org/jdk/compare/d89288e1...66b03944 - Changes: - all: https://git.openjdk.org/jdk/pull/11148/files - new: https://git.openjdk.org/jdk/pull/11148/files/718b4f64..66b03944 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11148=05 - incr: https://webrevs.openjdk.org/?repo=jdk=11148=04-05 Stats: 44518 lines in 696 files changed: 16672 ins; 16259 del; 11587 mod Patch: https://git.openjdk.org/jdk/pull/11148.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11148/head:pull/11148 PR: https://git.openjdk.org/jdk/pull/11148
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Fri, 18 Nov 2022 11:27:14 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Uh oh > > Aaaahhh, **that** bug. I've run into that before. I did attempt to fix it but > after wasting too many hours I gave up. :-( It seems to be a limitation in m4 > that I cannot understand how to get around. At some point, the string > literal, even though quoted inside the `[...]`, is expanded and parsed as m4 > macro expansion arguments. In theory, I should have been able to add an > additional layer of quoting and then un-quote it once it was past the > problematic point, but that did not work. > > I solved the problem by not solving it, and instead rephrased the message to > not need the comma. (Imho, this limitation actually improved the quality of > the descriptions, so it was not bad per se). > > But I agree that it is annoying to have such a limitation in > `UTIL_DEFUN_NAMED`. If you want to have a go at trying to solve it, please > do! I'll fully admit my shortcomings and state that trying to solve this > passes my knowledge and ability to manipulate m4. > > Or, maybe, you could add some documentation to `UTIL_DEFUN_NAMED` and > `UTIL_ARG_WITH`, saying that comma is not allowed in the values. @magicus I need some of your input on this: Typically when using AC_ARG_WITH one would surround the if block with an extra quote, for instance if we had `if ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]]` the code in m4 would have an extra set of brackets to quote it so m4 doesn't absolutely wreck the final script that we want to emit, such as `if [ ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]] ]` I'm about halfway through fixing the whole issue with commas, and I'm fairly certain I could lump the problem with quoting expansion in there too. My question though is: When it comes to parameters that take "predicate" code blocks as arguments (Such as CHECK_VALUE), should I have UTIL_ARG_NAMED handle the annoying issue of quoting entirely so the caller can freely pass `if ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]]` without having to worry at all, or should I make users still have to keep passing that extra level of quotation into things like IF_GIVEN to match what is already being done? - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19 [v5]
On Thu, 17 Nov 2022 21:46:45 GMT, Matias Saavedra Silva wrote: >> The -XX:+AutoCreateSharedArchive flag was implemented in JDK 19, however, >> this flag doesn't work across JDK 19 and 20. >> >> Expected: JDK 20 should recreate the specified CDS archive >> Actual: JDK 20 cannot recognize the archive file and gives up >> >> The new field from GenericCDSFileMapHeader is now in FileMapHeader. Verified >> with tier 1-4 tests. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Added newline to end of test Thank you for the feedback Eric, Calvin, and Ioi! - PR: https://git.openjdk.org/jdk/pull/11148
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v8]
On Tue, 22 Nov 2022 10:59:05 GMT, Christoph Langer wrote: >> Hm, the changes you made to the markdown file does not result in any changes >> in the generated html file. :( I fixed it by adding an additional empty line >> in the markdown file. The commit including both these fixes are here: >> https://github.com/magicus/jdk/commit/2844de1591ee8246dfb0fb5d19664b9db1d68c3a > > @magicus Thanks for fixing my enumeration. I recognized that you have > regenerated all the html files with a newer pandoc and I had to resolve with > master. I guess you'll need to regenerate with my current branch once more - > otherwise I think I'm breaking some of the new style. @RealCLanger Finally regenerated once more: [fcd20c73ed9f0a44c0614a54580c755a16387a6a](https://github.com/magicus/jdk/commit/fcd20c73ed9f0a44c0614a54580c755a16387a6a) - PR: https://git.openjdk.org/jdk/pull/3
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v8]
On Fri, 18 Nov 2022 16:32:08 GMT, Magnus Ihse Bursie wrote: >> Christoph Langer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix enumeration for html > > Hm, the changes you made to the markdown file does not result in any changes > in the generated html file. :( I fixed it by adding an additional empty line > in the markdown file. The commit including both these fixes are here: > https://github.com/magicus/jdk/commit/2844de1591ee8246dfb0fb5d19664b9db1d68c3a @magicus Thanks for fixing my enumeration. I recognized that you have regenerated all the html files with a newer pandoc and I had to resolve with master. I guess you'll need to regenerate with my current branch once more - otherwise I think I'm breaking some of the new style. - PR: https://git.openjdk.org/jdk/pull/3
Re: RFR: 8296904: Improve handling of macos xcode toolchain [v9]
> With this PR I'd like to make it easier to use dedicated installations of > Xcode on Mac OS for OpenJDK builds without having to switch the active Xcode > globally via `xcode-select`. > > I propose adding a new configure flag `--with-xcode-path` that takes the path > to an Xcode installation. If the option is set, this path is expanded to a > valid `TOOLCHAIN_PATH`. > > Furthermore, I fix detection of xcodebuild and correctly setting the sysroot > from the toolchain by moving `AC_SUBST(TOOLCHAIN_PATH)` before calling > `BASIC_SETUP_XCODE_SYSROOT` and honoring `TOOLCHAIN_PATH` in > `BASIC_SETUP_XCODE_SYSROOT` in the case when no devkit is specified. As I see > it, this is a viable fix, even if not introducing `--with-xcode-path`. Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into xcode-toolchain - Fix bullet list formatting in markdown and update html - Fix enumeration for html - Update html file - Add dots - Whitespace stuff - Minor polishing - Updating building.md doc - Merge branch 'master' into xcode-toolchain - Review suggestion - ... and 2 more: https://git.openjdk.org/jdk/compare/42c20374...2a5e4e44 - Changes: https://git.openjdk.org/jdk/pull/3/files Webrev: https://webrevs.openjdk.org/?repo=jdk=3=08 Stats: 86 lines in 3 files changed: 25 ins; 16 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/3.diff Fetch: git fetch https://git.openjdk.org/jdk pull/3/head:pull/3 PR: https://git.openjdk.org/jdk/pull/3
