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

extract: Get gcc args by using compile_commands.json #96

Merged
merged 3 commits into from
Mar 10, 2025

Conversation

marcosps
Copy link
Collaborator

Our make strategy worked well until we found the a new way of getting the gcc arguments used to compile a kernel source file. Recently it started to fail on newer codestreams, failing to find the proper source directory from the -obj directory.

Use this opportunity to finally implement the arguments finding using compile_commands.json that is packaged by kernel-livepatch-default-livepatch-devel rpm.

The paths are extactly the same being found using make, but the instead of the absolute paths to the source directory of the codestreams, the compile_commands.json used '../'. Replacing ../ by the source directory of the affected codestream fixes the problem.

Our make strategy worked well until we found the a new way of getting
the gcc arguments used to compile a kernel source file. Recently it started
to fail on newer codestreams, failing to find the proper _source_ directory from the
-obj directory.

Use this opportunity to finally implement the arguments finding using compile_commands.json
that is packaged by kernel-livepatch-default-livepatch-devel rpm.

The paths are extactly the same being found using make, but the instead of the
absolute paths to the source directory of the codestreams, the compile_commands.json
used '../'. Replacing ../ by the source directory of the affected codestream fixes
the problem.

Signed-off-by: Marcos Paulo de Souza <[email protected]>
@marcosps marcosps requested review from vmezzela and fgyanz February 28, 2025 14:53

logging.error(f"Couldn't find cmdline for {fname}. Aborting")
logging.error("Couldn't find cmdline for %s. Aborting", fname)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps I'd rather not use here "Aborting", since if we don't find the command-line we're looking for in the json we just use the old method, right?
We could just emit a warning instead of an error, like:

Suggested change
logging.error("Couldn't find cmdline for %s. Aborting", fname)
logging.warning("Couldn't find cmdline for %s in compile_commands.json, using get_make_cmd().", fname)

or something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can do that, but we won't expect the make approach to work either, since compile_commands.json is created for each compiled source file. We can do what you suggested, but I won't expect to have more value from it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you on this, my concern is just that as far as I understand we're not aborting here. Unless I'm missing something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ouch, that's true. My bad. Thanks a lot for catching this issue! I believe that I should create one test for it, since it's easy to catch such issue!

marcosps added 2 commits March 5, 2025 17:42
Instead of returning errors on logging just return exceptions. This
makes things more simple to understand.

Also, return an error with a file cannot be found on compile_commands.json,
since it means that we have an issue regarding the specific file.

Signed-off-by: Marcos Paulo de Souza <[email protected]>
Copy link
Collaborator

@vmezzela vmezzela left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

Copy link
Collaborator

@fgyanz fgyanz left a comment

Choose a reason for hiding this comment

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

Manually tested with CVE 2024-56648 (the one that triggered the bug) and works great! Thanks a lot!

@fgyanz fgyanz merged commit ef80d6e into SUSE:devel Mar 10, 2025
1 check passed
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