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

Only build necessary Sleigh components #613

Merged
merged 8 commits into from
Aug 22, 2022
Merged

Conversation

tetsuo-cpp
Copy link
Contributor

No description provided.

@tetsuo-cpp tetsuo-cpp marked this pull request as draft August 3, 2022 17:12
CMakeLists.txt Outdated Show resolved Hide resolved
@tetsuo-cpp tetsuo-cpp requested a review from ekilmer August 3, 2022 17:13
CMakeLists.txt Outdated Show resolved Hide resolved
@ekilmer
Copy link
Contributor

ekilmer commented Aug 3, 2022

Tests are failing because the spec files aren't being compiled/installed. There are some spec targets you can un-exclude from all to make sure they're built

@tetsuo-cpp
Copy link
Contributor Author

Tests are failing because the spec files aren't being compiled/installed. There are some spec targets you can un-exclude from all to make sure they're built

I'll wait until we figure out lifting-bits/sleigh#105 (comment) since this appears to be the same issue.

@tetsuo-cpp
Copy link
Contributor Author

Hmm, the build_linux test has a known failure. But the build_mac and Docker_Linux failures look to be actual logic changes in Sleigh. Perhaps something has happened with Thumb2 since that Ghidra commit that Remill is pinned to.

@tetsuo-cpp
Copy link
Contributor Author

I'll debug this when I get the chance. But just confirming, @ekilmer, is this how the variables should be set?

@ekilmer
Copy link
Contributor

ekilmer commented Aug 17, 2022

Hmmm. It doesn't look like the additional patches are being applied, so maybe that's what is causing failures?

[ 44%] Performing patch step for 'ghidrasource-populate'
Applying: Small improvements to C++ decompiler testing from CLI
Applying: Add include guards to decompiler C++ headers
Applying: Fix UBSAN errors in decompiler
Applying: Use `stroull` instead of `stroul` to parse address offsets
[ 55%] No configure step for 'ghidrasource-populate'

https://github.com/lifting-bits/remill/runs/7832387939?check_suite_focus=true#step:4:574

@tetsuo-cpp
Copy link
Contributor Author

Hmmm. It doesn't look like the additional patches are being applied, so maybe that's what is causing failures?

[ 44%] Performing patch step for 'ghidrasource-populate'
Applying: Small improvements to C++ decompiler testing from CLI
Applying: Add include guards to decompiler C++ headers
Applying: Fix UBSAN errors in decompiler
Applying: Use `stroull` instead of `stroul` to parse address offsets
[ 55%] No configure step for 'ghidrasource-populate'

https://github.com/lifting-bits/remill/runs/7832387939?check_suite_focus=true#step:4:574

Oh that's interesting. I'll look into that now.

@tetsuo-cpp
Copy link
Contributor Author

Ok, this issue is fixed in 75adcde.

I think somehow the spot where we do:

set(sleigh_ADDITIONAL_PATCHES "" CACHE STRING
  "The accepted patch format is git patch files, to be applied via git am. The format of the list is a CMake semicolon separated list.")

Is overwriting the user setting. When I did a rebuild, it seemed to apply the patches correctly. So perhaps it has something to do with the ordering of where the variable is defined and where we pull the Ghidra source?

@ekilmer
Copy link
Contributor

ekilmer commented Aug 18, 2022

Ah. Yeah I didn't see that it was originally a normal variable for the patches. Mixing and matching still doesn't make sense to me, so it's always good to match the variable types. Usually for something like this, it will be a cache variable you are trying to modify.

Is overwriting the user setting.

Hmmm. That shouldn't happen. Whatever the first call to set the cache variable is what takes precedence. The cache variable set only takes affect if the variable is undefined when it reaches that set. Did you start with a completely fresh build directory?

@tetsuo-cpp
Copy link
Contributor Author

tetsuo-cpp commented Aug 22, 2022

Did you start with a completely fresh build directory?

Yeah, I blew away the build each time. It's the same thing that was causing the failure in CI (which would have been a fresh build directory).

Anyhow, specifying CACHE has fixed it so I think this is good to go.

@tetsuo-cpp tetsuo-cpp marked this pull request as ready for review August 22, 2022 05:47
@tetsuo-cpp tetsuo-cpp requested a review from ekilmer August 22, 2022 05:55
Copy link
Contributor

@ekilmer ekilmer left a comment

Choose a reason for hiding this comment

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

LGTM!

@pgoodman pgoodman merged commit a973a4d into master Aug 22, 2022
@pgoodman pgoodman deleted the alex/sleigh-cmake-refactor branch August 22, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants