search

anakryiko / retsnoop

Investigate kernel error call stacks
BSD 2-Clause "Simplified" License
209 stars 34 forks source link

bpftool: Pass additional compile flags as EXTRA_CFLAGS, not CFLAGS #52

Closed qmonnet closed 1 year ago

qmonnet commented 1 year ago

Bpftool expects users to pass additional compilation flags via EXTRA_CFLAGS, not via CFLAGS. This is because when compiled from the kernel repo, CFLAGS are usually overwritten by the build system.

When we build retsnoop with CFLAGS=--static, the static flag is passed down to bpftool's Makefile. Then we hit an issue: --static is ignored for feature probing at build time, but taken into account for the build itself. Under certain conditions this results in a discrepency and bpftool's Makefile believes LLVM support is available (given it tested without the --static flag) whereas it is not set up for static builds.

Let's address this by passing retsnoop's CFLAGS via bpftool's EXTRA_CFLAGS, and resetting its CFLAGS. We must pass the CFLAGS to the "make" invocation through the environment, and not as a command line argument, to avoid overriding the CFLAGS values that we set inside of bpftool's Makefile.

Related: https://github.com/anakryiko/retsnoop/pull/51#issuecomment-1741953868

qmonnet commented 1 year ago

Bonus: I added a CI workflow to build retsnoop on pull requests, let me know if you prefer to discard it.

anakryiko commented 1 year ago

Awesome, thanks a lot for CI build tests. Adding arm64 would definitely be great. I also wonder if static vs dynamic build would be better to split, so they can go in parallel? But yeah, this is awesome, thanks!

qmonnet commented 1 year ago

I also wonder if static vs dynamic build would be better to split, so they can go in parallel?

I wondered too, but thought this would need to install the deps on a second runner and was not sure it was worth it. But yes, it would certainly go faster to run them in parallel.

anakryiko commented 1 year ago

btw, I just created a draft release using CI action (https://github.com/anakryiko/retsnoop/actions/runs/6385199025), it all worked well this time. I'll be using this for the next release, thanks for your help!

qmonnet commented 1 year ago

Great to hear! Did the workflow_dispatch work as you expected? Maybe I should use it too for bpftool

anakryiko commented 1 year ago

Yep, I had to add workflow_dispatch and it worked for master. I tried for v0.9.7 tag, but that failed because we don't have all the fixes. I'll give it another try when I'm ready for new release and let you know if something is not working :)

I actually couldn't figure out how to do release without workflow_dispatch. What was the anticipated way to do this?

qmonnet commented 1 year ago

I actually couldn't figure out how to do release without workflow_dispatch. What was the anticipated way to do this?

Before commit https://github.com/anakryiko/retsnoop/commit/ad63eba8efa0b8971092a210ff1eaf6950343520, the workflow should have triggered every time someone pushes a tag matching the pattern ** (meaning, any tag). On my fork, where I tested the workflow, you can see (under the Actions tab) that the objects associated to the events for the older runs of the workflow were tags, test05 and the like.

It should have created a draft release automatically for the tag associated with the push event. I'm not sure how it retrieves a tag from workflow_dispatch, by the way.