8352064: AIX: now also able to build static-jdk image with a statically linked launcher#24062
8352064: AIX: now also able to build static-jdk image with a statically linked launcher#24062JoKern65 wants to merge 5 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back jkern! A progress list of the required criteria for merging this PR into |
|
@JoKern65 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 89 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@JoKern65 The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
make/StaticLibs.gmk
Outdated
| # INFO := Generating export list for static libs, \ | ||
| # DEPS := $(STATIC_LIB_FILE), \ | ||
| # OUTPUT_FILE := $(STATIC_LIB_EXPORT_FILES), \ | ||
| # COMMAND := $(AR) $(ARFLAGS) -w $(patsubst %.exp, %, $@) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | sort -u >$@, \ |
There was a problem hiding this comment.
The problem with using SetupExecute here is using $@ variables in the command line. To get this to work you need to delay resolving such variables. It may work by adding an adequate number of extra $, but I'm not sure how many would be needed or if it would actually work. You could try this, but it's probably trickier to get right than just doubling:
| # COMMAND := $(AR) $(ARFLAGS) -w $(patsubst %.exp, %, $@) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | sort -u >$@, \ | |
| # COMMAND := $(AR) $(ARFLAGS) -w $$(patsubst %.exp, %, $$@) | $(GREP) -v '^\.' | $(AWK) '{print $$$$1}' | sort -u >$$@, \ |
You are probably better off using the explicit output file variable $(STATIC_LIB_EXPORT_FILES).
There was a problem hiding this comment.
Thanks Eric, this might help with the command, but my problem is the dependency. The rules to generate the export files is not called, because in the build output I cannot find any line 'Generating export list for static libs'. Can you help out here too?
There was a problem hiding this comment.
The problem seems to be that $(generate_export_list) is empty and I do not know why.
There was a problem hiding this comment.
Looks like this variable is misspelled (missing "S").
DEPS := $(STATIC_LIB_FILE), \
Also note that by default log level INFO is not printed. You would need to run the build with make LOG=info to see the message.
There was a problem hiding this comment.
Ups, thank you for giving me the 'S'. But nevertheless if I make with
make all LOG=info an $(info generate_export_list: $(generate_export_list)) still shows an empty list
There was a problem hiding this comment.
I did not get the SetupExecute to function, even if I strip it down to a simple example
$(eval $(call SetupExecute, generate_export_list, \
INFO := Generating export list for static libs, \
DEPS := /path/libjli.a, \
OUTPUT_FILE := /path/libjli.a.exp, \
COMMAND := $(AR) $(ARFLAGS) -w $$(patsubst %.exp, %, $$@) | $(GREP) -v '^\.' | $(AWK) '{print $$$$1}' | $(SORT) -u >$$@, \
))
$(java): $(STATIC_LIB_FILES) /path/libjli.a.exp
I still get
gmake[3]: *** No rule to make target '/path/libjli.a.exp', needed by '/path2/java'. Stop.
Why does make not recognize, that the rule is in my
call SetupExecute, generate_export_list,
macro?
There was a problem hiding this comment.
This is likely due to the missing subshell. There is an enhancement filed to fix this automatically but it has unfortunately not been prioritized: https://bugs.openjdk.org/browse/JDK-8233115
make/StaticLibs.gmk
Outdated
| STATIC_LIBS := -Wl,-bexpfull $(STATIC_LIB_FILES) $(addprefix -Wl$(COMMA)-bE:, $(STATIC_LIB_EXPORT_FILES)) | ||
| $(STATIC_LIB_EXPORT_FILES): $(STATIC_LIB_FILES) | ||
| $(AR) $(ARFLAGS) -w $(patsubst %.exp, %, $@) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | sort -u >$@ | ||
| #$(eval $(call SetupExecute, generate_export_list, \ |
There was a problem hiding this comment.
I think there are multiple issues at hand here.
-
There is a current bug in SetupExecute that requires you to execute any command with redirection in a subshell, that is, you need to wrap the COMMAND in
(...). -
STATIC_LIB_EXPORT_FILES is not a single file, it is a list of file. So what you have written above is not a single recipe, it is a multi-file recipe template. But it's weird, it will evaluate to like:
libjvm.so.exp libjava.so.exp libjli.so.exp: libjvm.so libjava.so libjli.so
which makes no sense, since you do not need to have a dependency to libjava.so libjli.so for libjvm.so.exp.
What you really want is to have a single rule per library that creates the corresponding exp file for that library, and which is dependent only on that library.
Or, possibly, you can just concatenate the export of all libraries into a single file. It will make the final linker command line simpler, since you would only need a single -Wl,-bE:<the export file>, but it is potentially worse for an incremental rebuild, since all native libraries need to be re-scanned for exported functions.
My preferred solution would to make this explicit by creating a loop over STATIC_LIB_FILES, and doing a SetupExecute for each such lib. Then you can get it down to something like:
$(foreach lib, $(STATIC_LIB_FILES), \
$(eval $(call SetupExecute, generate_export_list_$(lib),
INFO := Generating export list for $(lib), \
DEPS := $(lib), \
OUTPUT_FILE := $(lib).exp, \
COMMAND := ( $(AR) $(ARFLAGS) -w $(lib) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | sort -u > $(lib).exp ), \
) \
$(eval STATIC_LIB_EXPORT_FILES += $(lib).exp) \
)
make/StaticLibs.gmk
Outdated
| )) | ||
|
|
||
| $(java): $(STATIC_LIB_FILES) | ||
| $(java): $(STATIC_LIB_FILES) $(if $(call isTargetOs, aix), $(STATIC_LIB_EXPORT_FILES)) |
There was a problem hiding this comment.
This is just clunky. Leave the original line, and add:
ifeq ($(call isTargetOs, aix), true)
$(java): $(STATIC_LIB_EXPORT_FILES)
endif
|
I applied all of magnus proposals, but I still get |
|
I found the problem: I also get So I think point 3 of the PR description is solved, although I find the name The next point to work on is No. 2 of the PR description (3 |
|
Regaring point 2: the current solution works, and is acceptable, I think. The two excluded files only contain the dladdr() implementation, and this is implemented in hotspot, so no functionality is lost. Compacting this into just a single implementation is a separate issue, and need not be resolved now. Regarding the name of |
make/StaticLibs.gmk
Outdated
| STATIC_LIB_EXPORT_FILES := $(foreach lib, $(STATIC_LIB_FILES), $(lib).exp) | ||
| STATIC_LIBS := -Wl,-bexpfull $(STATIC_LIB_FILES) $(addprefix -Wl$(COMMA)-bE:, $(STATIC_LIB_EXPORT_FILES)) | ||
| $(foreach lib, $(STATIC_LIB_FILES), \ | ||
| $(eval $(call SetupExecute, generate_export_list_$(notdir $(lib)), \ | ||
| INFO := Generating export list for $(notdir $(lib)), \ | ||
| DEPS := $(lib), \ | ||
| OUTPUT_FILE := $(lib).exp, \ | ||
| COMMAND := ( $(AR) $(ARFLAGS) -w $(lib) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | $(SORT) -u > $(lib).exp ), \ | ||
| )) \ |
There was a problem hiding this comment.
To better align with how we store the targets of Setup calls, can you please change this to:
| STATIC_LIB_EXPORT_FILES := $(foreach lib, $(STATIC_LIB_FILES), $(lib).exp) | |
| STATIC_LIBS := -Wl,-bexpfull $(STATIC_LIB_FILES) $(addprefix -Wl$(COMMA)-bE:, $(STATIC_LIB_EXPORT_FILES)) | |
| $(foreach lib, $(STATIC_LIB_FILES), \ | |
| $(eval $(call SetupExecute, generate_export_list_$(notdir $(lib)), \ | |
| INFO := Generating export list for $(notdir $(lib)), \ | |
| DEPS := $(lib), \ | |
| OUTPUT_FILE := $(lib).exp, \ | |
| COMMAND := ( $(AR) $(ARFLAGS) -w $(lib) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | $(SORT) -u > $(lib).exp ), \ | |
| )) \ | |
| STATIC_LIBS := -Wl,-bexpfull $(STATIC_LIB_FILES) $(addprefix -Wl$(COMMA)-bE:, $(STATIC_LIB_EXPORT_FILES)) | |
| $(foreach lib, $(STATIC_LIB_FILES), \ | |
| $(eval $(call SetupExecute, generate_export_list_$(notdir $(lib)), \ | |
| INFO := Generating export list for $(notdir $(lib)), \ | |
| DEPS := $(lib), \ | |
| OUTPUT_FILE := $(lib).exp, \ | |
| COMMAND := ( $(AR) $(ARFLAGS) -w $(lib) | $(GREP) -v '^\.' | $(AWK) '{print $$1}' | $(SORT) -u > $(lib).exp ), \ | |
| )) \ | |
| $(eval STATIC_LIB_EXPORT_FILES += $(lib).exp) \ |
magicus
left a comment
There was a problem hiding this comment.
The build changes look good to me now. You need to have someone review the hotspot AIX changes as well.
TheRealMDoerr
left a comment
There was a problem hiding this comment.
I'm not really familiar with it, but I have some code style requests.
src/hotspot/os/aix/loadlib_aix.cpp
Outdated
| if (pgmpath[0] == 0) { | ||
| procentry64 PInfo; | ||
| PInfo.pi_pid = ::getpid(); | ||
| if ( 0 == ::getargs(&PInfo, sizeof(PInfo), (char*)pgmpath,PATH_MAX) && *pgmpath ) { |
There was a problem hiding this comment.
Coding style: spaces after ( and before ) are uncommon, but we should have one before PATH_MAX.
I think hotspot style guide compliant would be:
if (0 == ::getargs(&PInfo, sizeof(PInfo), (char*)pgmpath, PATH_MAX) && *pgmpath != 0)
src/hotspot/os/aix/loadlib_aix.cpp
Outdated
| pgmpath[PATH_MAX] = '\0'; | ||
| pgmbase = strrchr(pgmpath, '/'); | ||
| if (pgmbase) { | ||
| pgmbase +=1; |
There was a problem hiding this comment.
Maybe add a space: "+= 1"?
src/hotspot/os/aix/loadlib_aix.cpp
Outdated
| if ( 0 == ::getargs(&PInfo, sizeof(PInfo), (char*)pgmpath,PATH_MAX) && *pgmpath ) { | ||
| pgmpath[PATH_MAX] = '\0'; | ||
| pgmbase = strrchr(pgmpath, '/'); | ||
| if (pgmbase) { |
There was a problem hiding this comment.
Better: if (pgmbase != nullptr)
src/hotspot/os/aix/loadlib_aix.cpp
Outdated
| lm->data_len = ldi->ldinfo_datasize; | ||
|
|
||
| lm->path = g_stringlist.add(ldi->ldinfo_filename); | ||
| if (pgmbase && 0 == strcmp(pgmbase, ldi->ldinfo_filename)) { |
TheRealMDoerr
left a comment
There was a problem hiding this comment.
LGTM. Maybe @MBaesken wants to take a look at the AIX code, too?
|
Regarding BitmapToYXBandedRectangles from splashscreen, couldn't we just rename it to avoid clashes/issues with the other one from awt with the same name but other signature? |
| lm->data_len = ldi->ldinfo_datasize; | ||
|
|
||
| lm->path = g_stringlist.add(ldi->ldinfo_filename); | ||
| if (pgmbase != nullptr && 0 == strcmp(pgmbase, ldi->ldinfo_filename)) { |
There was a problem hiding this comment.
Maybe better strcmp(...) == 0 this is what we use almost everywhere in the HS codebase .
Indeed we could. Note that the same issue apply to all unix platforms. It's sort of a hack -- the core problem is that we cannot hide non-exported symbols in static libraries. Solving that is the long-term goal, but it is tricky on several platforms. It could make sense to try to unify the two versions of BitmapToYXBandedRectangles as well; they are similar but not identical (but I can't really tell why). |
|
Seems we use already a bit of functionality from common/awt in the splashscreen lib maybe it would make sense to centralize the BitmapToYXBandedRectangles there and reuse it from there? |
|
/integrate |
|
Going to push as commit d8c2f59.
Your commit was automatically rebased without conflicts. |
Maybe. The client team has traditionally been very conservative about doing clean-up changes like that, so you might be entering an uphill battle if you want to do that. |
After "JDK-8339480: Build static-jdk image with a statically linked launcher" AIX was not able to build the new target. Therefore with "JDK-8345590 AIX 'make all' fails after JDK-8339480" the new target was disabled again.
Now with this change we can enable the statically linked launcher target again.
There are 3 things to do.
dladdr(). Because this API does not exist on AIX it is implemented based on theloadquery()API. Unfortunately, this API does only return the name of the executable, but not its path. Beforehand this was no problem, because we asked for a loaded library, for which the API provides the path. But now we are asking for the executable itself.dladdr()is differently implemented three times in the openjdk code. In the static case I supressed now the usage of the additional modules containing version two and three. I know this shouldn't be the final version. Magnus mentioned that they have discussed from time to time to have a "basic JDK utility lib" that can be shared between hotspot and the JDK libraries. I think this is a good idea for the future, but far beyond the scope of this PR. The second best thing Magnus proposed is to only have thedladdr()functionality in Hotspot and then export it. Let's see how the community decides.whole-archive, I had to force the export of all symbols by creating export files containing all symbols of the static libs. I introduced the new rule for the export file generation as "raw" make recipes. Magnus claimed to use theSetupExecute. Unfortunately I was not able to make it function. So I still have my "raw" solution in place, but my last try withSetupExecuteas comment beneath. Help is welcome.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24062/head:pull/24062$ git checkout pull/24062Update a local copy of the PR:
$ git checkout pull/24062$ git pull https://git.openjdk.org/jdk.git pull/24062/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24062View PR using the GUI difftool:
$ git pr show -t 24062Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24062.diff
Using Webrev
Link to Webrev Comment