lifting-bits / remill

Library for lifting machine code to LLVM bitcode
Apache License 2.0
1.22k stars 142 forks source link

Only build necessary Sleigh components #613

Closed tetsuo-cpp closed 1 year ago

ekilmer commented 1 year ago

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 commented 1 year ago

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 https://github.com/lifting-bits/sleigh/pull/105#issuecomment-1204385903 since this appears to be the same issue.

tetsuo-cpp commented 1 year ago

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 commented 1 year ago

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

ekilmer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.