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

[MuAlphaOmegaEpsilon]

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.

[MuAlphaOmegaEpsilon]

Rebased to more recent main branch, updated comment

[aaron.ballman]

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

35

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

177–180

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–14645

[MuAlphaOmegaEpsilon]

Update code after review and add tests

[aaron.ballman]

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

clang/test/Sema/warn-unused-parameters.c

27–29 ↗

(On Diff #399793)

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

[MuAlphaOmegaEpsilon]

In D116778#3244233, @aaron.ballman wrote:

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

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

[MuAlphaOmegaEpsilon]

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

[• Quuxplusone]

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

290–292 ↗

(On Diff #400078)

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) { ; }

[aaron.ballman]

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 ↗

(On Diff #400078)

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]

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. :)

[MuAlphaOmegaEpsilon]

Update warning amount on CHECK-unused check

[MuAlphaOmegaEpsilon]

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

290–292 ↗

(On Diff #400078)

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!

[MuAlphaOmegaEpsilon]

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]

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?

[MuAlphaOmegaEpsilon]

In D116778#3255414, @aaron.ballman wrote:

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 <[e-mail]>

Thank you very much Aaron! 😀

[aaron.ballman]

In D116778#3259189, @MuAlphaOmegaEpsilon wrote:

In D116778#3255414, @aaron.ballman wrote:

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 <[e-mail]>

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!

[MuAlphaOmegaEpsilon]

In D116778#3276421, @aaron.ballman wrote:

In D116778#3259189, @MuAlphaOmegaEpsilon wrote:

In D116778#3255414, @aaron.ballman wrote:

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 <[e-mail]>

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! 😀

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

bool isOverrideMethod(const FunctionDecl *Function) {
   if (const auto *MD = dyn_cast<CXXMethodDecl>(Function))
     return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
   return false;
 }
 } // namespace
 
 void UnusedParametersCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(isDefinition(), hasBody(stmt()), hasAnyParameter(decl()))
           .bind("function"),
       this);
 }
 
 template <typename T>
 static CharSourceRange removeNode(const MatchFinder::MatchResult &Result,
                                   const T *PrevNode, const T *Node,
                                   const T *NextNode) {
for (const CallExpr *Call : Indexer->getFnCalls(Function))
     if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
       MyDiag << removeArgument(Result, Call, ParamIndex);
 }
 
 void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function");
   if (!Function->hasWrittenPrototype() || Function->isTemplateInstantiation())
     return;
+  if (Function->hasAttrs())
+    for (const clang::Attr *A : Function->getAttrs())
+      if (A->getParsedKind() == Attr::AT_Naked)
+        return;
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Function))
     if (Method->isLambdaStaticInvoker())
       return;
   for (unsigned I = 0, E = Function->getNumParams(); I != E; ++I) {
     const auto *Param = Function->getParamDecl(I);
     if (Param->isUsed() || Param->isReferenced() || !Param->getDeclName() ||
         Param->hasAttr<UnusedAttr>())
       continue;

clang/lib/Sema/SemaDecl.cpp

if (FD) {
         WP.disableCheckFallThrough();
 
       // MSVC permits the use of pure specifier (=0) on function definition,
       // defined at class scope, warn about this non-standard construct.
       if (getLangOpts().MicrosoftExt && FD->isPure() && !FD->isOutOfLine())
         Diag(FD->getLocation(), diag::ext_pure_function_definition);
 
       if (!FD->isInvalidDecl()) {
-        // Don't diagnose unused parameters of defaulted or deleted functions.
-        if (!FD->isDeleted() && !FD->isDefaulted() && !FD->hasSkippedBody())
+        bool FDHasNakedAttr{false};
+        if (FD->hasAttrs())
+          for (const clang::Attr *A : FD->getAttrs())
+            if (A->getParsedKind() == Attr::AT_Naked) {
+              FDHasNakedAttr = true;
+              break;
+            }
+        // Don't diagnose unused parameters of defaulted, deleted or naked
+        // functions.
+        if (!FD->isDeleted() && !FD->isDefaulted() && !FD->hasSkippedBody() &&
+            !FDHasNakedAttr)
           DiagnoseUnusedParameters(FD->parameters());
         DiagnoseSizeOfParametersAndReturnValue(FD->parameters(),
                                                FD->getReturnType(), FD);
 
         // If this is a structor, we need a vtable.
         if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(FD))
           MarkVTableUsed(FD->getLocation(), Constructor->getParent());
         else if (CXXDestructorDecl *Destructor =