cheri_compartment: Warn if return type is void or return value is unused #47
Conversation
|
Thanks for doing this. We are in the middle of rebasing our changes on a newer LLVM, so I’d like to hold off merging until that’s done. The warning would be better if it is explicit about why it’s a bad idea, rather than saying that it’s unsupported. I’d recommend something like: void return on a cross-compartment call makes it impossible for callers to detect failure. A fixit hint that changed the type to int and changed return statements to return 0 (and adds one at the end of the function) would be ideal. |
|
Thank you. I'll look at FixItHint. |
|
This comment is for documentation.
|
|
I need to adapt an existing function, I will reimplement it where it's needed but I also opened an issue to know if it's acceptable to directly modify the function: llvm/llvm-project#118290. |
|
Here the following error messages:
Do you agree with the diagnosis levels (warn, note, ...) ? Don't forget to comment on capitalisation or ending the messages with dot (or not). Just asking to not turn this PR into a Nicaea council. I plan to add this (corrected) description of the messages to the issue description. |
|
Sounds good! |
|
Sounds good to me (and it's a useful diagnostic to have; thanks!) |
|
This PR is almost finished but there is some parts that depend on modifying the behavior of some core clang class (FunctionDecl::getReturnTypeLoc). I'm working on a patch for trunk llvm and waiting for review. This modification is needed for the FixItHint to replace void by int. I think it's going to take 2 more weeks. |
|
I don't mind carrying the |
|
You're probably going to need to cherry-pick this onto the new llvm-17 based branch. |
v01dXYZ
force-pushed
the
cheriot-compartment-warn-if-return-void-or-unused
branch
from
de97656 to
2eac335
Compare
|
The CI timed out. Is there any way to relaunch it without pushing ? |
v01dXYZ
force-pushed
the
cheriot-compartment-warn-if-return-void-or-unused
branch
2 times, most recently
from
7795792 to
5c89ebb
Compare
|
@v01dXYZ I'm not sure if you can merge this of it you need me to do it. If the latter, please rebase it and then ping me to merge it. |
The current implementation has a little shortcoming: the warning message for void return type is shown twice because cheri_compartment is a function type attribute (cf SemaType.cpp) that is added to both the attribute list of the declarator and the one of the function type.
v01dXYZ
force-pushed
the
cheriot-compartment-warn-if-return-void-or-unused
branch
from
c2d7cc6 to
b8c007f
Compare
|
It seems it's pretty easy to run out of credits with the Cirrus CI free plan. |
|
@v01dXYZ Getting a report from @nwf that this is causing linker errors in cheriot-rtos. I'm going to revert these changes for now. I won't have time to look into the failure in detail in the next two days. If you figure out what's wrong in the meantime, go ahead and re-PR it. Otherwise I'll help figure out the problem when I'm around again. |
|
The full link error is: The |
|
I suspect a strange behavior from xmake. My reasoning: compilation should fail as there is the Werror flag set. There are some bits of code that have a void return type or ignore the return value of a compartment call. I wonder also if it creates a empty file instead of no file, this file is then ignored by the linker. |
|
I made some time to look into it, and I think I found it. I'll push a PR for you to review soon. |
|
At least it forced me to compile cheriot-rtos tests. I'll add a line in my local cirrus.yml for doing that since it only requires installing xmake which is debian/Ubuntu available |
Be careful as there we were hit by some issues in various versions of xmake and the version available in debian / ubuntu repos may not work correctly. At present the devcontainer Dockerfile is installing a version from a ppa but that may only work on Ubuntu. |
Fixes #34.