[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+        return x;
+}
+
----------------
erichkeane wrote:
> craig.topper wrote:
> > erichkeane wrote:
> > > craig.topper wrote:
> > > > erichkeane wrote:
> > > > > erichkeane wrote:
> > > > > > craig.topper wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > craig.topper wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > 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`.
> > > > > > > > > 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);                                                              
> > > > > > > > > }  
> > > > > > > > > ```
> > > > > > > > Yes, I would expect that to warn.
> > > > > > > It looks like handleAlwaysInlineAttr only gets called once so it doesn't get called after instantiation.
> > > > > > 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.
> > > > > 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.
> > > > What I do need to add?
> > > 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.
> > Can I do that as a separate patch? Fixing the crash seems like a net improvement.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146089/new/

https://reviews.llvm.org/D146089