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

CMake: Add initial support for ddrgen #3308

Merged
merged 3 commits into from Apr 3, 2019
Merged

CMake: Add initial support for ddrgen #3308

merged 3 commits into from Apr 3, 2019

Conversation

dnakamura
Copy link
Contributor

No description provided.

@dnakamura dnakamura mentioned this pull request Dec 13, 2018
Open
14 tasks
@dnakamura
Copy link
Contributor Author

Most if not all of the core logic should be in place. Just need to add documentation and squash commits

@dnakamura dnakamura changed the title WIP: add support for ddr from cmake CMake: Add initial support for ddrgen Feb 21, 2019
@DanHeidinga
Copy link
Contributor

@charliegracie @keithc-ca Can you review this PR?

@charliegracie
Copy link
Contributor

@genie-omr build all

1 similar comment
@youngar
Copy link
Member

youngar commented Feb 28, 2019

@genie-omr build all

keithc-ca
keithc-ca reviewed Mar 1, 2019
View reviewed changes
cmake/modules/OmrDDRSupport.cmake Outdated
set(DDR_BIN_DIR "${CMAKE_CURRENT_BINARY_DIR}/${DDR_TARGET_NAME}")
set(DDR_TARGETS_LIST "${DDR_BIN_DIR}/targets.list")
set(DDR_MACRO_INPUTS_FILE "${DDR_BIN_DIR}/macros.list")
set(DDR_TOOLS_EXPORT "${omr_BINARY_DIR}/ddr/tools/DDRTools.cmake")
Copy link
Member

Choose a reason for hiding this comment

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

Where does DDRTools.cmake come from?

nit: inconsistent indentation

cmake/modules/ddr/DDRSetStub.cmake.in
TARGET "${target}"
SOURCE_DIR "${OPT_SOURCE_DIR}"
OUTPUT_ANNOTATED project_annt_hdr
#TODO need to add pre-include files
Copy link
Member

Choose a reason for hiding this comment

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

We should just fix header files to no 'pre-include' is necessary.

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 will do that for any of our code, however I would like to keep this in as a convenience for any downstream users.

cmake/modules/ddr/DDRSetStub.cmake.in Outdated Show resolved Hide resolved
cmake/modules/ddr/GenerateStub.cmake Outdated Show resolved Hide resolved
cmake/modules/ddr/cmake_ddr.awk Outdated Show resolved Hide resolved
ddr/test/CMakeLists.txt Outdated Show resolved Hide resolved
ddr/tools/CMakeLists.txt
@@ -21,3 +21,5 @@

add_subdirectory(blob_reader)
add_subdirectory(ddrgen)

export(TARGETS omr_blob_reader omr_ddrgen FILE "DDRTools.cmake")
Copy link
Member

Choose a reason for hiding this comment

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

Where does DDRTools.cmake come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This statement generates the DDRTools.cmake file

Copy link
Member

Choose a reason for hiding this comment

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

I still don't get it; export declares that this project produces DDRTools.cmake - my question is how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/v3.12/command/export.html

Export targets from the build tree for use by outside projects.
Create a file filename that may be included by outside projects to import targets from the current project’s build tree.

Copy link
Member

Choose a reason for hiding this comment

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

Let me rephrase: what defines the content of that file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its internally defined by cmake. When you include it it will define an imported target with the appropriate paths, etc. Here is a sample from when we export hookgen/tracegen
https://gist.github.com/dnakamura/24ecce3061474162e890f72838551619

Copy link
Member

Choose a reason for hiding this comment

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

Certainly cmake doesn't know anything about hookgen or tracegen: where is the information that tells it what to put in that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omr/tools/CMakeLists.txt

Line 38 in ded9f8c

export(TARGETS hookgen tracemerge tracegen FILE "ImportTools.cmake")

i believe it just puts the location where the targets are built

doc/ddr/CMake.md Outdated Show resolved Hide resolved
omr/startup/CMakeLists.txt Outdated Show resolved Hide resolved
@DanHeidinga
Copy link
Contributor

@keithc-ca Can you re-review this? It looks like @dnakamura pushed a new version

keithc-ca
keithc-ca suggested changes Mar 21, 2019
View reviewed changes
Copy link
Member

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

There are also a number of unanswered questions from my last review.

cmake/modules/OmrDDRSupport.cmake Outdated Show resolved Hide resolved
cmake/modules/ddr/DDRSetStub.cmake.in Outdated Show resolved Hide resolved
cmake/modules/ddr/GenerateStub.cmake Outdated Show resolved Hide resolved
cmake/modules/ddr/cmake_ddr.awk Show resolved Hide resolved
ddr/test/CMakeLists.txt Outdated Show resolved Hide resolved
ddr/tools/CMakeLists.txt
@@ -21,3 +21,5 @@

add_subdirectory(blob_reader)
add_subdirectory(ddrgen)

export(TARGETS omr_blob_reader omr_ddrgen FILE "DDRTools.cmake")
Copy link
Member

Choose a reason for hiding this comment

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

I still don't get it; export declares that this project produces DDRTools.cmake - my question is how?

@dnakamura dnakamura force-pushed the cmake_ddr branch 2 times, most recently from 6695556 to bbe9052 Compare March 21, 2019 18:33
@dnakamura
Copy link
Contributor Author

Oops. appears i forgot to push last round of changes. @keithc-ca I believe i got them all

@dnakamura
Copy link
Contributor Author

@keithc-ca poke

@youngar youngar self-assigned this Mar 30, 2019
@youngar
Copy link
Member

youngar commented Apr 1, 2019

@genie-omr build all

@youngar
Copy link
Member

youngar commented Apr 1, 2019

Arm failed due to infrastructure issues, while linux x86-64 and zos 390-64 failed with:

14:25:51  CMake Error at cmake/modules/OmrDDRSupport.cmake:93 (file):
14:25:51    Error evaluating generator expression:
14:25:51  
14:25:51      $<TARGET_OBJECTS:omrgc_tracegen>
14:25:51  
14:25:51    The evaluation of the TARGET_OBJECTS generator expression is only suitable
14:25:51    for consumption by CMake.  It is not suitable for writing out elsewhere.
14:25:51  Call Stack (most recent call first):
14:25:51    gc/CMakeLists.txt:363 (target_enable_ddr)
14:25:51

@dnakamura
CMake: Add initial support for ddrgen
d29eb53 
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
@dnakamura
Remove usage of string(JOIN ...)
c5d125c 
This is not implemented in cmake 3.4. Instead create omr_join utility
function with the same behaviour

Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
@dnakamura
CMake: add EARLY_SOURCE_EVAL option to target_enable_ddr
169003e 
Specifying this option forces the targets sources to be evaluated immediately,
rather than at generate time. This is required to filter out use of
$<TARGET_OBJECTS:>, which is not allowed on older versions of cmake

Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
@youngar
Copy link
Member

youngar commented Apr 2, 2019

@genie-omr build all

@youngar youngar merged commit b4b1008 into eclipse:master Apr 3, 2019
@dnakamura dnakamura deleted the cmake_ddr branch October 27, 2020 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca requested changes

@charliegracie charliegracie Awaiting requested review from charliegracie

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@youngar youngar Awaiting requested review from youngar

Assignees

@youngar youngar

Labels
cmake comp:diagnostic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dnakamura @DanHeidinga @charliegracie @youngar @keithc-ca @mstoodle