[Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[craig.topper]

Summary

It's possible for getCalleeDecl() to return a null pointer.

This was encountered by a user of our downstream compiler.

The case involved a DependentScopeDeclRefExpr.

Since this seems to only be for a warning diagnostic, I skipped
the diagnostic check if it returned null. But mabye there's a
different way to fix this.

[craig.topper]

Add a test case

[erichkeane]

clang/test/Sema/attr-alwaysinline.cpp

36

Can you add a test that shows that we warn on instantiation? This shouldn't be a dependent declrefexpr when instantiated.

Additionally, this would make sure that we're properly propoagating always_inline.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

36

Should this warn

template<int D> [[gnu::noinline]]                                                
int bar(int x) {                                                                 
    if constexpr (D > 1)                                                         
        [[clang::always_inline]] return bar<D-1>(x + 1);                         
    else                                                                         
        return x;                                                                
}                                                                                
                                                                                 
int baz(int x) {                                                                 
  return bar<5>(x);                                                              
}

[erichkeane]

clang/test/Sema/attr-alwaysinline.cpp

36

Yes, I would expect that to warn.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

36

It looks like handleAlwaysInlineAttr only gets called once so it doesn't get called after instantiation.

[erichkeane]

clang/lib/Sema/SemaStmtAttr.cpp

236–237

Same fix + a test probably needed here too.

clang/test/Sema/attr-alwaysinline.cpp

32

Also, I note the 'if constexpr' branch is unnecessary to reproduce this.

36

Hmm... thats unfortunate. That means we're perhaps not instantiating it correctly. I'll take some time to poke around as I get a chacne.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

32

Yes, but I didn't bother to figure out what happened if the template recursivley instantiated without bound.

[erichkeane]

clang/test/Sema/attr-alwaysinline.cpp

36

Ok, it looks like SemaStmt.cpp's BuildAttributedStmt needs to handle the attributes. We should some day do that automatically for a majority o f attributes, but for now, just adding the call there is sufficient.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

36

What I do need to add?

[craig.topper]

Fix the same bug for noinline attribute

[erichkeane]

clang/test/Sema/attr-alwaysinline.cpp

36

You should be able to follow what is already there, but I suspect you just need to call some sort of 'handle' function for those to do this. There is one there already youc an crib off of.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

36

Can I do that as a separate patch? Fixing the crash seems like a net improvement.

[erichkeane]

clang/test/Sema/attr-alwaysinline.cpp

36

I'd prefer they be done together: They are a situation where you cannot properly test either patch without the other one being fixed, so I believe they are intrinsically linked.

[craig.topper]

clang/test/Sema/attr-alwaysinline.cpp

36

I thought not crashing on code that doesn't need to warn would still be an improvement.

The warning that's being lost doesn't seem all that serious. It's just telling that an alwaysinline statement attribute has priority over a noinline function attribute.

We don't warn for this

void foo();

void bar() {
  [[clang::always_inline]] foo();
}

If foo's definition is in another translation unit, the always_inline can't happen without LTO. So foo is effectively noinline without LTO.

[erichkeane]

Please add the test cases requested, plus a Release Note. Otherwise LGTM.

clang/test/Sema/attr-alwaysinline.cpp

36

The warning that's being lost doesn't seem all that serious. It's just telling that an alwaysinline statement attribute has priority over a noinline function attribute.

Isn't it saying that the statement diagnostic is losing to the function attribute?

As far as that example, I wouldn't expect the diagnostic to show there, as you mentioned.

That said, I thought this was going to be a bit more trivial than it is, I thought this was going to be a mild drive-by. There needs to be some level of extracting the getCallExprs checks in both cases into Sema, and have that called upon instantiation.

I think we're OK now with this, but PLEASE make sure to put a number of tests in for the template cases with a "FIXME".

[craig.topper]

Recommitted at e82d2e8c11119ca34bcccb8fef67a8727543a978

[erichkeane]

Please add a release note as requested.

[craig.topper]

In D146089#4202037, @erichkeane wrote:

Please add a release note as requested.

To highlight the fix or highlight the missing warning? Or both?

[erichkeane]

To highlight the fix. See the rest of our release notes.

[craig.topper]

In D146089#4202374, @erichkeane wrote:

To highlight the fix. See the rest of our release notes.

4743c03ca8fcb61b8fa4022c38cf93cf55d7f6fd

clang/lib/Sema/SemaStmtAttr.cpp

static Attr *handleNoInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
   if (!CEF.foundCallExpr()) {
     S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
         << A;
     return nullptr;
   }
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
     const Decl *Decl = CallExpr->getCalleeDecl();
-    if (Decl->hasAttr<AlwaysInlineAttr>() || Decl->hasAttr<FlattenAttr>())
+    if (Decl &&
+        (Decl->hasAttr<AlwaysInlineAttr>() || Decl->hasAttr<FlattenAttr>()))
       S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
           << A << (Decl->hasAttr<AlwaysInlineAttr>() ? 0 : 1);
   }
 
   return ::new (S.Context) NoInlineAttr(S.Context, A);
 }
 
 static Attr *handleAlwaysInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
static Attr *handleAlwaysInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
   if (!CEF.foundCallExpr()) {
     S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
         << A;
     return nullptr;
   }
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
     const Decl *Decl = CallExpr->getCalleeDecl();
-    if (Decl->hasAttr<NoInlineAttr>() || Decl->hasAttr<FlattenAttr>())
+    if (Decl && (Decl->hasAttr<NoInlineAttr>() || Decl->hasAttr<FlattenAttr>()))
       S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
           << A << (Decl->hasAttr<NoInlineAttr>() ? 2 : 1);
   }
 
   return ::new (S.Context) AlwaysInlineAttr(S.Context, A);
 }
 
 static Attr *handleMustTailAttr(Sema &S, Stmt *St, const ParsedAttr &A,

clang/test/Sema/attr-alwaysinline.cpp

void foo() {
   [[clang::always_inline]] noinline_fn(); // expected-warning {{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
   [[clang::always_inline]] flatten_fn();  // expected-warning {{statement attribute 'always_inline' has higher precedence than function attribute 'flatten'}}
 
   [[gnu::always_inline]] bar();         // expected-warning {{attribute is ignored on this statement as it only applies to functions; use '[[clang::always_inline]]' on statements}}
   __attribute__((always_inline)) bar(); // expected-warning {{attribute is ignored on this statement as it only applies to functions; use '[[clang::always_inline]]' on statements}}
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning {{'always_inline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template<int D>
+int foo(int x) {
+    if constexpr (D > 1)
+        [[clang::always_inline]] return foo<D-1>(x + 1);
+    else
+        return x;
+}
+
+// FIXME: This should warn that always_inline statement attribute has higher
+// precedence than the noinline function attribute.
+template<int D> [[gnu::noinline]]
+int bar(int x) {
+    if constexpr (D > 1)
+        [[clang::always_inline]] return bar<D-1>(x + 1);
+    else
+        return x;
+}

clang/test/Sema/attr-noinline.cpp

void foo() {
   [[clang::noinline]] flatten_fn(); // expected-warning {{statement attribute 'noinline' has higher precedence than function attribute 'flatten'}}
   [[clang::noinline]] noinline_fn();
 
   [[gnu::noinline]] bar(); // expected-warning {{attribute is ignored on this statement as it only applies to functions; use '[[clang::noinline]]' on statements}}
   __attribute__((noinline)) bar(); // expected-warning {{attribute is ignored on this statement as it only applies to functions; use '[[clang::noinline]]' on statements}}
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template<int D>
+int foo(int x) {
+    if constexpr (D > 1)
+        [[clang::noinline]] return foo<D-1>(x + 1);
+    else
+        return x;
+}
+
+// FIXME: This should warn that noinline statement attribute has higher
+// precedence than the always_inline function attribute.
+template<int D> [[clang::always_inline]]
+int bar(int x) {
+    if constexpr (D > 1)
+        [[clang::noinline]] return bar<D-1>(x + 1);
+    else
+        return x;
+}