Skip to content

Only build necessary Sleigh components#613

Merged
pgoodman merged 8 commits into
masterfrom
alex/sleigh-cmake-refactor
Aug 22, 2022
Merged

Only build necessary Sleigh components#613
pgoodman merged 8 commits into
masterfrom
alex/sleigh-cmake-refactor

Conversation

@tetsuo-cpp

Copy link
Copy Markdown
Contributor

No description provided.

@tetsuo-cpp tetsuo-cpp marked this pull request as draft August 3, 2022 17:12
Comment thread CMakeLists.txt Outdated
@tetsuo-cpp tetsuo-cpp requested a review from ekilmer August 3, 2022 17:13
Comment thread CMakeLists.txt Outdated
@ekilmer

ekilmer commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@ekilmer

ekilmer commented Aug 17, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

ekilmer commented Aug 18, 2022

Copy link
Copy Markdown
Contributor

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

tetsuo-cpp commented Aug 22, 2022

Copy link
Copy Markdown
Contributor Author

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

@ekilmer ekilmer left a comment

Copy link
Copy Markdown
Contributor

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