-
Notifications
You must be signed in to change notification settings - Fork 444
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
Fix missing declaration of custom externs in the generated eBPF/uBPF header file. #4644
Conversation
The documentation seems to suggest that this is by design, and Agree that the filename check used in this PR is brittle, but I don't know if there is a better way... Is there any testdata for eBPF externs? |
The failing tests are here: https://github.com/p4lang/p4c/actions/runs/8862986990/job/24336526254 The problem is that, while the My fix simply adds a declaration in the header file to prevent the warning from occurring. The definition is still in the It would be great if we had golden files which show the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying @fruffy. I notice in that test output that only the ubpf extern tests are failing, not ebpf - do we know why?
I agree that emitting the declarations is a good step forwards, the suggested -include
method feels like a bit of a hack (imho).
As mentioned in my previous comment, I don't think that discriminating based on the source filename is robust. We're already missing ebpf_model.p4
and xdp_model.p4
here. If we don't want to emit declarations for "standard" externs which are already defined, perhaps a better solution would be to filter based on a list of such standard externs? Any other suggestions?
I will give this a shot. |
Yeah, I wondered about that too. It must be related to the compile order, but I can't identify the difference. |
229af1a
to
cb0a5c2
Compare
@thomascalvert-xlnx I implemented a technique to filter based on method names instead. I also created #4645 because I ran into a bunch of issues. For example, |
cb0a5c2
to
4986811
Compare
a033738
to
782f630
Compare
782f630
to
333a083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for looking at this Fabian!
Agree it's hard to work on the eBPF backend due to the tangled dependencies & inheritance with other backends (ubpf, tc, psa). Regarding the progTarget
variable you refer to, as far as I can tell it is in fact only really useful to the PSA backend (sub-backend?).
88cac13
to
915199d
Compare
915199d
to
0378e15
Compare
I do not understand the Ubuntu 20.04 failure. The tests passes on Ubuntu 18.04 and Ubuntu 22.04 but not here. The output doesn't help either. @thomascalvert-xlnx any intuition. Do we really need to declare every function |
It looks like the verifier is rejecting the program with the following error:
There seems to be a similar issue reported here: xdp-project/xdp-tutorial#38 As far as I can tell it's an issue with specific tool versions - could be a mismatch between libbpf versions of the OS vs this repo? Or could be some problem with the debug info, try dumping it as suggested in the link above? |
47ec5db
to
95778d6
Compare
#2407 could help here, but I never implemented it. Unfortunate. |
01ff92f
to
95778d6
Compare
I give up. I do not know what else to try. I will just ignore this test on Ubuntu 20.04. FYI this is the output of the failing test, I do not see anything suspicious:
|
@thomascalvert-xlnx I disabled method signature generation for eBPF and only enabled it for uBPF. Until we fix the way we load eBPF programs (or move on from Ubuntu 20.04) I do not see a better way to do this. Please take a second look. |
@thomascalvert-xlnx I will merge this tomorrow if you have no further comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, we had a bank holiday here and this thread fell out of my cache!
LGTM now, thank you @fruffy :)
Fixes a warning that occurs when compiling eBPF/uBPF programs with a custom, user-defined extern. The extern has not been declared, which causes GCC to spit out
warning: implicit declaration of function
.Because there are also method declarations in
core.p4
and the corresponding include file I need to add a filter based on source information. I do not know whether there is a better approach to this.I also added the error type, which is the same as the enum type. I also could add
Type_String
, but it has unclear semantics on width.@thomascalvert-xlnx