Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tool configuration #2106

Merged
merged 3 commits into from
Jun 14, 2018
Merged

Tool configuration #2106

merged 3 commits into from
Jun 14, 2018

Conversation

keithc-ca
Copy link
Contributor

  • use C/C++ compilers more consistently
  • remove 'use_ld_to_link' property
  • adjust use of 'isCPlusPlus' option
    -- it should never be conditional
    -- it has no meaning for static libraries

@keithc-ca keithc-ca force-pushed the tool-config branch 2 times, most recently from a502c13 to 2c6a8e1 Compare June 6, 2018 20:43
@keithc-ca keithc-ca mentioned this pull request Jun 6, 2018
Merged
pshipton
pshipton reviewed Jun 6, 2018
View reviewed changes
runtime/makelib/targets.mk.aix.inc.ftl Outdated
endif
UMA_DLL_LINK_FLAGS += -qmkshrobj=0 -Wl,-brtl -Wl,-G -Wl,-bernotok -Wl,-bnoentry -lm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is -p 0 removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-p 0 is an option understood by makeC++SharedLib_r; this PR changes aix*.spec to use the C compiler: the equivalent is -qmkshrobj=0.

pshipton
pshipton reviewed Jun 6, 2018
View reviewed changes
runtime/makelib/targets.mk.aix.inc.ftl Outdated
endif
UMA_DLL_LINK_FLAGS += -qmkshrobj=0 -Wl,-brtl -Wl,-G -Wl,-bernotok -Wl,-bnoentry -lm
UMA_DLL_LINK_FLAGS += -Wl,-bE:$(UMA_TARGET_NAME).exp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why -bE: instead of -E? Was -E a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above, as we switch to linking with the C compiler, we have to speak its language.
The -E option of makeC++SharedLib_r maps to -Wl,-bE:$(UMA_TARGET_NAME).exp.

@keithc-ca
Copy link
Contributor Author

UMA Tools

UMA uses several kinds of tools via these properties:

UMA Property Usage
uma_make_cmd_cc The C compiler command
uma_make_cmd_cxx The C++ compiler command
uma_make_cmd_interp_gcc The BytecodeInterpreter compiler command
uma_make_cmd_ppc_gcc_cxx The BytecodeInterpreter compiler command (for PPC)
uma_make_cmd_dll_ld The linker for shared objects (with no C++ dependencies)
uma_make_cmd_cxx_dll_ld The linker for shared objects (with C++ dependencies)
uma_make_cmd_exe_ld The linker for executables (with no C++ dependencies)
uma_make_cmd_cxx_exe_ld The linker for executables (with C++ dependencies)

Currently, these tools are essentially independent.

The following tools are discovered at OpenJDK configuration time:

Tool Name Usage
CC The C compiler command
CXX The C++ compiler command

The plan is to adjust the OpenJ9 *.spec and module.xml files so this mapping consistently applies:

UMA Property Value
uma_make_cmd_cc $(CC)
uma_make_cmd_cxx $(CXX)
uma_make_cmd_interp_gcc $(CXX)
uma_make_cmd_ppc_gcc_cxx $(CXX)

All of the UMA properties specifying linker commands will be adjusted to use $(CC) or $(CXX) as appropriate.

To accomplish this:

  • each of uma_make_cmd_cc, uma_make_cmd_dll_ld and uma_make_cmd_exe_ld must name a C compiler (not a C++ compiler), otherwise linking may create unwanted references to C++ support libraries
  • none of the properties can make unreasonable assumptions about the capabilities of tools named
    by other properties (e.g. $(CC) can't be expected to know anything about C++).
  • artifacts in module.xml files that specify the UMA option isCPlusPlus should assume that implies a dependency on compiler C++ support libraries

DanHeidinga
DanHeidinga reviewed Jun 7, 2018
View reviewed changes
runtime/makelib/targets.mk.aix.inc.ftl Outdated
endif
# Link options like '-brtl', '-G', etc. must be prefixed by '-Wl,' to make xlc_r pass them through.
UMA_DLL_LINK_FLAGS += -qmkshrobj -Wl,-brtl -Wl,-G -Wl,-bernotok -Wl,-bnoentry -lm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the -bmap:$(UMA_TARGET_NAME).map get deliberately dropped (why) or accidentally lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll restore the map creation.

@DanHeidinga
Copy link
Member

As discussed today, we need to ensure that this change doesn't cause us to link the c++ standard library.

@keithc-ca keithc-ca mentioned this pull request Jun 11, 2018
Closed
@keithc-ca keithc-ca force-pushed the tool-config branch 2 times, most recently from 2c3cdc8 to de5492d Compare June 11, 2018 21:00
@keithc-ca keithc-ca changed the title WIP: Tool configuration Tool configuration Jun 11, 2018
@keithc-ca
Copy link
Contributor Author

I think this is good to go. I wrote a script to use ldd to compare the dependencies of executables and shared libraries before and after this change: no extra references to C++ libraries.

pshipton
pshipton reviewed Jun 12, 2018
View reviewed changes
runtime/makelib/targets.mk.aix.inc.ftl Outdated
UMA_DLL_LINK_FLAGS += -bmap:$(UMA_TARGET_NAME).map
UMA_DLL_LINK_FLAGS += -bE:$(UMA_TARGET_NAME).exp
UMA_LINK_LIBRARIES += -lc_r -lC_r -lm -lpthread
UMA_SYS_LINK_PATH := -L/usr/lib/threads
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment to explain the origins of this

@pshipton
Copy link
Member

jenkins test sanity

keithc-ca added 3 commits June 12, 2018 17:44
@keithc-ca
Use C/C++ compilers more consistently
a0780db 
* remove 'use_ld_to_link' property
* always use C compiler for linking
  -- modules that require dynamically link C++ shared libraries
     must do so explicitly

Signed-off-by: Keith W. Campbell <[email protected]>
@keithc-ca
Adjust 'isCPlusPlus' option usage
dc4a7aa 
* it should never be conditional
* it has no meaning for static libraries

Signed-off-by: Keith W. Campbell <[email protected]>
@keithc-ca
Adjust makefiles
5369b59 
* correct references to xlc C++ library
* remove configuration related to UMA_USING_LD_TO_LINK

Signed-off-by: Keith W. Campbell <[email protected]>
@keithc-ca
Copy link
Contributor Author

Comment added as requested.

@pshipton
Copy link
Member

jenkins test sanity

@pshipton
Copy link
Member

jenkins test sanity xlinuxlargeheap jdk10

@keithc-ca
Copy link
Contributor Author

Who can help fix this?

Error: java.io.IOException: No space left on device

@pshipton
Copy link
Member

Who can help fix this?

Created #2161

@DanHeidinga
Copy link
Member

As discussed today, we need to ensure that this change doesn't cause us to link the c++ standard library.

@keithc-ca Have you validated that this doesn't link the C++ stdlib anywhere we didn't before?

@keithc-ca
Copy link
Contributor Author

@DanHeidinga Yes, I verified that there are no new references to C++ shared libraries.
See #2106 (comment).

@pshipton pshipton merged commit e71c261 into eclipse-openj9:master Jun 14, 2018
@keithc-ca keithc-ca deleted the tool-config branch June 14, 2018 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga left review comments

@pshipton pshipton pshipton approved these changes

Assignees
No one assigned
Labels
comp:build comp:gc comp:vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keithc-ca @DanHeidinga @pshipton