Add testcase for finding crash offset #4112
Conversation
|
CC @Cl4sm |
for more information, see https://pre-commit.ci
Test Results 86 files ±0 86 suites ±0 1h 46m 24s ⏱️ + 5m 30s For more details on these errors, see this check. Results for commit 77f6455. ± Comparison against base commit f10b145. ♻️ This comment has been updated with latest results. |
|
Fix the linting issues and I will click merge! |
tests/test_crash_offset.py
Outdated
|
|
||
| binary_path = tempfile.mktemp() | ||
|
|
||
| subprocess.run(["gcc", program_path, "-o", binary_path, "-fno-stack-protector"]) |
There was a problem hiding this comment.
Is this test only intended to run on linux? This is gonna fail nightlies on macos and windows
There was a problem hiding this comment.
It honestly may be a better idea to pre-compile binaries and put them in the binaries repo rather than compiling this every time the test is run, especially since nowhere does angr explicitly depend on gcc,
There was a problem hiding this comment.
Why is this going to fail on macos/windows?
There was a problem hiding this comment.
Would you really rather ship a bunch of precompiled binaries for this? The very small source code being included here makes the testcase much more readable.
Can we make angr testcases explicitly depend on gcc? I don't think angr needs to, just angr testcases.
There was a problem hiding this comment.
I suppose gcc is aliased to clang by default on mac, but on windows it is not standard. I would not expect gcc to be in a user's path on windows. angr users msvc on windows.
There was a problem hiding this comment.
The prevailing wisdom on the angr team is that it's better to ship binaries so we don't have to deal with testcase failures when gcc updates :)
for more information, see https://pre-commit.ci
This functionality currently works. I want to guarantee that it continues to work. If the current approach should be changed at all, please let me know.