Skip to content

Conversation

wdullaer
Copy link
Contributor

Moves the code to generate go bindings to its own dedicated package and export the necessary functions to use it in bpf2go.

This change will make it possible to reuse this code in other tools, without having to compile the C programs.

The behaviour of bpf2go is unchanged.

Two additional changes were made:

  1. The 'Module' parameter has been made an input to the code generation. Otherwise embedding the bindings package in another module would lead to incorrect import statements in the generated code. I fail to see the value of the 'debug' lookup, the only valid value is "github.com/cilium/ebpf", but maybe there are some cilium internal build constraints, so I left it configurable.
  2. The function which transforms type names to go identifiers has been updated to deal with all caps names. These shouldn't be present in C code, but appear when using rust. The change ensures the field names in the bindings are idiomatic Go field names.

This approach was taken after reading the feedback in #1142

@wdullaer wdullaer requested a review from a team as a code owner May 30, 2024 14:21
@wdullaer wdullaer force-pushed the export-bindings-generation branch from 7f2f5cd to 094ddd7 Compare May 30, 2024 14:22
@wdullaer wdullaer force-pushed the export-bindings-generation branch from 094ddd7 to d0043ec Compare June 6, 2024 19:03
Moves the code to generate go bindings to its own dedicated package
and export the necessary functions to use it in bpf2go.

This change will make it possible to reuse this code in other tools,
without having to compile the C programs.

The behaviour of bpf2go is unchanged.

Two additional changes were made:
1. The 'Module' parameter has been made an input to the code generation.
  Otherwise embedding the bindings package in another module would lead to
  incorrect import statements in the generated code.
  I fail to see the value of the 'debug' lookup, the only valid value is
  "github.com/cilium/ebpf", but maybe there are some cilium internal build
  constraints, so I left it configurable.
2. The function which transforms type names to go identifiers has been updated
  to deal with all caps names. These shouldn't be present in C code, but
  appear when using rust. The change ensures the field names in the bindings
  are idiomatic Go field names.

Signed-off-by: Wouter Dullaert <[email protected]>
@wdullaer wdullaer force-pushed the export-bindings-generation branch from d0043ec to 36cdcba Compare June 7, 2024 08:09
@wdullaer
Copy link
Contributor Author

wdullaer commented Jun 7, 2024

I made the Identifier function configurable. It will use the existing function as default.

I wanted to apply stronger normalization to the produced go identifiers, but this would result in a breaking change.
This removes the changes to the Identifier function that I mentioned in the original PR comment.

@mheese
Copy link

mheese commented Jun 11, 2024

Stumbling across this because I also need a solution for what's described in #1142 . I was about to fork and work on this myself, and then I saw that there is this PR here already 👍 ... However, I can also see that there is already #1448 as well. Personally, I like this solution here better because I think it's the right idea to move this to a dedicated package and keep it outside of the command/main package.

@ti-mo and @lmb , it looks like some time has passed since the original discussion in #1142. This (non-breaking) change to bpf2go would help unblock others (like myself) without us needing to fork and maintain this repository ourselves.

@lmb
Copy link
Collaborator

lmb commented Jun 21, 2024

@wdullaer sorry it took so long. I took your code as a base and extended it to cover most of the interesting parts of bpf2go: #1494 I undid the two changes you proposed. I think the module should not be configurable, because we are generating a binding against the API of the ebpf library. The identifier change makes sense to me but didn't have any tests. If the other PR lands we you can propose it again!

@lmb lmb closed this Jun 21, 2024
@wdullaer
Copy link
Contributor Author

I think you misunderstood why I made module configurable.
If you use the Generate function outside of the cilium/ebpf module, the current code (which is replicated in the new PR) will return the identifier of the calling module. The template uses the module as the location of the cilium/ebpf module though. This means it will spit out code that can't compile when used outside of cilium/ebpf.
The code as it lives in #1448 fails to actually solve the problem I intended to tackle in this PR because of that.

I agree that it shouldn't be configurable, it should be hardcoded to current fallback value of github.com/cilium/ebpf.

I also don't understand why you do removed the Ident function again.
It is already a function in the current codebase, and represents an opinion on how go identifiers should be generated from BTF identifiers.
I'm not sure how you would envision testing this: the actual logic is in the code you pass in. The change I made is trivial: read the function from the arg struct rather than the current package.

@lmb
Copy link
Collaborator

lmb commented Jun 24, 2024

I also don't understand why you do removed the Ident function again.

Couple of reasons:

  • There is no reason not to break it out into a separate change.
  • I think it makes sense to at least write a test which exercises that the intended output ends up in the template.
  • We should consider adding validation that the returned string is a valid Go identifier.

@wdullaer
Copy link
Contributor Author

Fair enough on the separate change.

For the validation of the returned string: since the input is a function, you can't guarantee you will exhaustively test the full state space. It is the responsibility of the caller to make sure that the function logic makes sense.
The compiler will check the output for you when trying to compile the result. Trying to do this in line in ebpf2go as well won't add much I think.

If you're open to accepting the feature, I can supply another PR once #1448 merges.

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