Page MenuHomePhabricator

[clang-tidy][clang] Don't trigger unused-parameter warnings on naked functions
ClosedPublic

Authored by MuAlphaOmegaEpsilon on Jan 6 2022, 4:23 PM.

Details

Summary

This commit checks if a function is marked with the naked attribute and,
if it is, will silence the emission of any unused-parameter warning.

Inside a naked function only the usage of basic ASM instructions is expected. In this context the parameters can actually be used by fetching them according to the underlying ABI. Since parameters might be used through ASM instructions, the linter and the compiler will have a hard time understanding if one of those is unused or not, therefore no unused-parameter warning should ever be triggered whenever a function is marked naked.

Diff Detail

Unit TestsFailed

TimeTest
1,010 msx64 debian > SanitizerCommon-tsan-x86_64-Linux.Linux::decorate_proc_maps.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -funwind-tables -ldl -g /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/decorate_proc_maps.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/decorate_proc_maps.cpp.tmp

Event Timeline

MuAlphaOmegaEpsilon requested review of this revision.Jan 6 2022, 4:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 6 2022, 4:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Rebased to more recent main branch, updated comment

MuAlphaOmegaEpsilon edited the summary of this revision. (Show Details)Jan 8 2022, 3:08 PM

Thanks for the fix! Can you be sure to add test coverage for both clang-tidy and Clang to demonstrate the behavior change?

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
33–34

Something along these lines should work instead (you'll have to reformat though).

178–181

I think this should be done using matchers instead of from check() if possible so that we get better memoization. See comment above.

clang/lib/Sema/SemaDecl.cpp
14635–14639

Update code after review and add tests

The changes look good aside from the precommit CI failing. Can you investigate?

clang/test/Sema/warn-unused-parameters.c
30–32

It looks like the precommit CI is failing because this wasn't updated.

The changes look good aside from the precommit CI failing. Can you investigate?

Absolutely, I'm already giving a look at it! :)

Fix failing test by updating clang/test/Sema/warn-unused-parameters.c expected warnings amount

Quuxplusone added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
290–292

In C++, I would expect the programmer to fix the (correct) warning simply by eliminating the unused parameter names:

[[gnu::naked]] int nakedFunction(int, float, const char *) { ; }
__attribute__((naked)) void nakedFunction(int, int) { ; }

CI still seems unhappy:

Failed Tests (1):
  Clang :: Sema/warn-unused-parameters.c
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
290–292

I wouldn't expect that consistently. I'd expect the programmer to see "unused parameter, oh, I should remove that" at least some significant percentage of the time, but with mixed results.

I think silencing the warning is a reasonable behavior in the presence of the attribute. Naked functions are pretty strange beasts to begin with.

MuAlphaOmegaEpsilon added a comment.EditedJan 14 2022, 1:39 PM

The warning would be correct if the compiler could actually tell the parameter is unused, but at the moment it cannot, as far as I know.

I stumbled upon this thing myself, and there are a few ways to silence the warning:
-removing parameters' names
-marking every single parameter as unused

The first one makes the function less readable, and forces me to add extra declarations or documentation to explain to the user the purpose of each parameter.

The second one is quite cumbersome to implement and it clutters the code quite a bit.

I consider these as sub-optimal scenarios, given that whatever I choose I only get drawbacks in order to silence a warning that I think shouldn't really be there in the first place. :)

Update warning amount on CHECK-unused check

MuAlphaOmegaEpsilon marked 3 inline comments as done.Jan 14 2022, 2:12 PM
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
290–292

I replied to this stream with a basic comment instead of using an inline one...
I'm sorry, I just figured out how this platform works with these drafts!

Let me know if I should rebase this onto the latest main branch, at the moment the Windows build is passing but the Debian build is not, failing at compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/decorate_proc_maps.cpp for no immediately apparent reason.

aaron.ballman accepted this revision.Jan 19 2022, 10:08 AM

LGTM! The CI failure is finally down to just an unrelated one (yay?). Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

This revision is now accepted and ready to land.Jan 19 2022, 10:08 AM
MuAlphaOmegaEpsilon added a comment.EditedJan 20 2022, 11:27 AM

LGTM! The CI failure is finally down to just an unrelated one (yay?). Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Great! 🎉
I've never committed on the LLVM repository, so I don't know the requirements. If it doesn't bother you, feel free to commit on my behalf using the following:
MuAlphaOmegaEpsilon <tommasobonvicini@gmail.com>

Thank you very much Aaron! 😀

aaron.ballman closed this revision.Jan 27 2022, 8:41 AM

LGTM! The CI failure is finally down to just an unrelated one (yay?). Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Great! 🎉
I've never committed on the LLVM repository, so I don't know the requirements. If it doesn't bother you, feel free to commit on my behalf using the following:
MuAlphaOmegaEpsilon <tommasobonvicini@gmail.com>

Thank you very much Aaron! 😀

Sorry about the delay in getting this landed (it slipped beneath my radar). I've commit on your behalf in ccce1a03c9ce9c3917b310097c89e39bb68527e2. Thank you for the fix!

LGTM! The CI failure is finally down to just an unrelated one (yay?). Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Great! 🎉
I've never committed on the LLVM repository, so I don't know the requirements. If it doesn't bother you, feel free to commit on my behalf using the following:
MuAlphaOmegaEpsilon <tommasobonvicini@gmail.com>

Thank you very much Aaron! 😀

Sorry about the delay in getting this landed (it slipped beneath my radar). I've commit on your behalf in ccce1a03c9ce9c3917b310097c89e39bb68527e2. Thank you for the fix!

No problem, I wasn't in a hurry! :)

Thank you very much again Aaron! 😀