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.

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

Use NASM assembler on x86-64 Windows and update NASM set-up process on OSX and xLinux makefiles #3459

Merged
merged 6 commits into from
Jan 15, 2019

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Oct 26, 2018

Depends on: #3293 , #3461 , #3493

This PR builds on top of the changeset in #3293 , and contains commits that are specific to x86_64 Linux, and will be rebased once #3293 is merged.

Issue: #3148

Signed-off-by: Nazim Uddin Bhuiyan [email protected]

@nbhuiyan nbhuiyan force-pushed the nasm-windows branch 5 times, most recently from a042a29 to 3524643 Compare January 10, 2019 17:05
@nbhuiyan nbhuiyan changed the title WIP: Use NASM assembler on x86-64 Windows Use NASM assembler on x86-64 Windows Jan 10, 2019
@nbhuiyan
Copy link
Member Author

@0xdaryl Ready for review

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 10, 2019

Jenkins test sanity all jdk11

1 similar comment
@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 11, 2019

Jenkins test sanity all jdk11

@nbhuiyan
Copy link
Member Author

The CI build failures on win64 so far has been due to how -I include path search directives are handled in NASM 2.13, and the fact that we are using cygwin for the jenkins builds. Prior to NASM version 2.14, -I include paths provided in the command line would not undergo any further processing, and would simply be concatenated with the filename specified in the %include preprocessor directive, requiring us to have a trailing \ or / in the include file search paths.

When running NASM through Cygwin, it appears as though the full paths enclosed in quotes for the -I command line arguments get processed into a version without the trailing slash, which results in being unable to open the file to include due to the missing path separator between the file name and the containing directory when NASM naively concatenates them. Having absolute paths enclosed in quotes is a good idea in the situation that a parent directory contains a space in its name.

This will not be a problem in NASM version 2.14 and newer, as the -I option semantics have been vastly improved and we are no longer required to ensure a trailing slash in the -I paths. If want to use the full path in -I, which is not a requirement, even a slightly older version of NASM will be unable to build the assembly files. Given that NASM 2.14 was released very recently, perhaps it is too soon to make that a requirement when there is a workaround to it.

The workaround (currently in this PR) that I have been able to come up with supports NASM version 2.14 as well as the previous releases. This involves not enclosing the -I paths in quotes, and using relative paths with a trailing slash from compiler directory to the include file search directories instead of absolute paths. That way, we do not have to worry about the possibility of spaces in a parent directory resulting in issues. The target file and the source files are still going to be enclosed in quotes without any issues. This will allow us to build the JIT using both Cygwin and native Windows shells.

I would still like the transition to NASM v2.14 to happen soon, as it would clean up some hacks that were needed in CMake, but with this approach, we can make the transition smoother as it would work across the 2 versions.

Currently, the %include directives for Windows have been ifdef'd out as I experimented with different command formats. If we can agree on my current implementation, I will apply the same approach to macOS and xLinux as well and clean this up.

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 12, 2019

Jenkins test sanity all jdk11

@0dvictor
Copy link
Contributor

0dvictor commented Jan 14, 2019

I don't think requiring NASM 2.14 is unreasonable. The workaround being applies has serve limitations: 1. it is quite common seeing spaces in the path, for example the Desktop folder in Windows 7 is under "Documents and Settings"; 2. search paths of .inc files are different from the old PASM ones and other C/C++ files. In addition, none of existing code requires any early version of NASM and hence I don't think requiring the latest version is too soon. As a reference, we introduced the requirement of Clang 7 only one month after it was released.

However, if upgrading/installing the required NASM 2.14 cannot make the deadline of a targeted release, I accept this workaround with two one extra notes in the building instructions and/or release notes:

  1. Mark NASM 2.14 as a requirement;
  2. Document the limitation that no space can exist in build path.

@nbhuiyan
Copy link
Member Author

@0dvictor

Spaces can exist in the build path, since the paths being provided with the -I option are relative, and are the following:

NASM_INCLUDES=\
    ../oti \
    ../compiler \
    ../compiler/x/runtime \
    ../compiler/x/amd64/runtime

Relative -I paths are used when building other components of OpenJ9 as well, so we are not adding any new constraints.

@0dvictor
Copy link
Contributor

@nbhuiyan Thank you for clarification. Now I see spaces in the paths to build should be okay.

* Added NASM_ASSEMBLER platform variable for win64
* Updated toolcfg/common.mk and rules/filetypes.mk to handle NASM

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
Also, NASM_ASSEMBLER is now defined on win64.

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
Contains various minor fixes. Also, file paths need to use
backslashes on Windows. This is because NASM does not deal with
how OS file paths can differ, and simply tries searching for files
to include by prepending the include paths provided in the command
line to the file to include until it is found.

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
Specifies NASM v2.13.03 and newer as a requirement for building
OpenJ9 on Windows.

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
NASM build rule use its own variables (such as NASM_OBJ_FORMAT,
NASM_INCLUDES, etc) that are set up in toolcfg/gnu/common.mk. This
change makes the build process similar to how NASM is set up on
Windows.

-I paths have been updated to not require specifying part of the
directory paths to .inc files to include.

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
@nbhuiyan nbhuiyan changed the title Use NASM assembler on x86-64 Windows Use NASM assembler on x86-64 Windows and update NASM set-up process on OSX and xLinux makefiles Jan 14, 2019
@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 14, 2019

Jenkins test sanity all jdk11

Signed-off-by: Nazim Uddin Bhuiyan <[email protected]>
@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 14, 2019

Jenkins test sanity all jdk11

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 15, 2019

Jenkins test sanity plinux,aix jdk11

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 15, 2019

Sanity tests have passed, as well as additional stress tests performed within IBM. This PR is also needed to enable #4249. Merging.

@0xdaryl 0xdaryl merged commit 4d40830 into eclipse-openj9:master Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants