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

A number of compiler sources assume that omr is a subdirectory of openj9 #3725

Closed
dnakamura opened this issue Nov 19, 2018 · 12 comments
Closed

Comments

@dnakamura
Copy link
Contributor

There are a number of places in the compiler sources which have includes of the form #include "omr/compiler/foo/bar.hpp" This assumes that omr exists as a sub-directory of openj9. This prevents us from performing out-of-tree builds. (see also eclipse-omr/omr#3189).

Offenders I have found so far:
compiler/runtime/Runtime.hpp
compiler/il/ILProps.hpp
compiler/il/ILOpCodeProperties.hpp
compiler/x/amd64/codegen/TreeEvaluatorTable.hpp

@DanHeidinga
Copy link
Member

@mstoodle @fjeremic Will this cause a problem with the way the namespacing and search path design the JIT uses?

@mstoodle
Copy link
Contributor

doesn't that path just assume that the directory that contains omr is on the compiler's include search path?

@mstoodle
Copy link
Contributor

mstoodle commented Nov 19, 2018

if you remove omr from these paths, then nothing in OpenJ9 will be able to specifically include the omr version of that header file...

For example, if you remove omr from openj9/runtime/compiler/runtime/Runtime.hpp then you'll cause an infinite include....The reason omr is there is to specially include OMR's compiler/runtime/Runtime.hpp

@mstoodle
Copy link
Contributor

mstoodle commented Nov 19, 2018

If we remove the omr then we'll need tor rename some of the files to smurf them with, e.g., OMR

Addendum: I did not add this comment to suggest we go down this path; i do not like this solution even though it is used within OMR itself.

@fjeremic
Copy link
Contributor

I think we introduced the omr/ include prefixes because of #1992 (comment) by @mstoodle. The omr/ include assumes the contents of the runtime/ directory in OpenJ9 is a sibling to all of OMR, i.e. the directory looks like this:

/runtime$ tree -d -L 1
.
├── bcutil
├── bcverify
├── cassume
├── cfdumper
├── clear_vmi
├── cmake
├── codert_vm
├── compiler
├── cuda
...
├── omr // This is the entire OMR repository

@dnakamura
Copy link
Contributor Author

Reworking the headers, so that the OMR version is named OMR<some_header>.hpp as is done with many of the other headers appears to work fine. Id be willing to open those PRs if people are alright with that solution

dnakamura added a commit to dnakamura/openj9 that referenced this issue Nov 19, 2018
For files which have an "OMR" prefix we are guaranteed not to have file
name conflicts in OpenJ9 and as such we can omit the "omr/" prefix in
the include path.

Issue eclipse-openj9#3725

Signed-off-by: Devin Nakamura <[email protected]>
@fjeremic
Copy link
Contributor

Reworking the headers, so that the OMR version is named OMR<some_header>.hpp as is done with many of the other headers appears to work fine. Id be willing to open those PRs if people are alright with that solution

@0xdaryl thoughts?

@mstoodle
Copy link
Contributor

Doesn't the OpenJ9 build system already need to know where the omr repository is located? Why can't the directory containing OMR just be a configurable setting and passed in on the IPATH which makes the compiler layering solution a more or less consistent mechanism?

(for the record and to make it clear, I hate the idea to smurf file names with the project name in the project even though we still do it in places).

@mstoodle
Copy link
Contributor

and, by the way, I think this issue is misnamed :)

dnakamura added a commit to dnakamura/openj9 that referenced this issue Dec 12, 2018
This allows disambiguation with the Runtime.hpp from omr, so we dont need to
specify the full path in include lines

Issue eclipse-openj9#3725

Signed-off-by: Devin Nakamura <[email protected]>
@DanHeidinga
Copy link
Member

@dnakamura What's the next step for this issue?

@dnakamura
Copy link
Contributor Author

just #4928

@dnakamura
Copy link
Contributor Author

Closing as last PR merged

dnakamura added a commit to dnakamura/omr that referenced this issue Jun 20, 2019
dnakamura added a commit to dnakamura/omr that referenced this issue Jul 9, 2019
dnakamura added a commit to dnakamura/omr that referenced this issue Jul 17, 2019
kobinau pushed a commit to kobinau/omr that referenced this issue Sep 11, 2019
Charlmzz pushed a commit to Charlmzz/omr that referenced this issue Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants