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

[BPF] report Invalid usage of the XADD return value" elegantly #92742

Merged
merged 1 commit into from
May 20, 2024

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented May 20, 2024

Previously report_fatal_error is used for reporting something goes wrong in the backend, but this is confusing because report_fatal_error basically means there are something unexpected & crashed in the backend.

So, turn this "crash" into an elegant error reporting. After this patch, clang can diagnose it:

bpf-crash.c:4:30: error: Invalid usage of the XADD return value
    4 | u32 next_event_id() { return __sync_fetch_and_add(&GLOBAL_EVENT_ID, 1); }
    |                              ^
1 error generated.

Previously `report_fatal_error` is used for reporting something goes
wrong in the backend, but this is confusing because `report_fatal_error`
basically means there are something unexpected & crashed in the backend.

So, turn this "crash" into an elegant error reporting.
After this patch, clang can diagnose it:

    bpf-crash.c:4:30: error: Invalid usage of the XADD return value
        4 | u32 next_event_id() { return __sync_fetch_and_add(&GLOBAL_EVENT_ID, 1); }
        |                              ^
    1 error generated.
@inclyc inclyc linked an issue May 20, 2024 that may be closed by this pull request
@eddyz87
Copy link
Contributor

eddyz87 commented May 20, 2024

The doc for diagnose says:

This function returns, ..., so the caller should leave the compilation process in a self-consistent state, even though the generated code need not be correct.

In this case, we will have a register used in some later instructions, this does not seem to violate any IR-level constraints, so this change should be fine as far as I understand.

@inclyc
Copy link
Member Author

inclyc commented May 20, 2024

In this case, we will have a register used in some later instructions, this does not seem to violate any IR-level constraints, so this change should be fine as far as I understand.

Actually if there are some errors generated, there are no binary files emitted at all. I suppose this will not affect end-users.

@inclyc inclyc merged commit c648663 into main May 20, 2024
6 checks passed
@inclyc inclyc deleted the users/inclyc/check-xadd-return branch May 20, 2024 17:57
@eddyz87
Copy link
Contributor

eddyz87 commented May 20, 2024

Actually if there are some errors generated, there are no binary files emitted at all. I suppose this will not affect end-users.

Yes, code generation stops if diagnose reports errors, I was curious if this change could lead to some incorrect machine code that leads to further error reports and it looks like it can't. So all should be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid usage of the XADD return value
2 participants