Don't warn on returning the address of a label from a statement expression

[rnk]

Summary

There isn't anything inherently wrong with returning a label from a
statement expression. In practice, the Linux kernel uses this pattern to
materialize PCs.

Fixes PR38569

[lebedev.ri]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7872–7874 ↗

(On Diff #160913)

Why completely drop the diagnostic just because it is undesired in linux code?
Why not just add an -Wreturn-stack-address diag option instead, and disable it if undesired?

[rnk]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7872–7874 ↗

(On Diff #160913)

There's just no use for it. There is no actual lifetime issue here. Just because a label goes out of scope doesn't invalidate it for use with some future execution of the scope. Labels aren't variables, we should never have had this check in the first place, IMO.

[rsmith]

There is no guarantee that you can use an address-of-label value from one function invocation in another invocation of the same function. GCC's documentation says it's the user's own problem to prevent inlining and cloning if the program requires an address-of-label extension to always produce the same value for multiple invocations of the function. It might make sense to suppress the warning in the case where the function is __attribute__((noinline)), though.

clang/lib/Sema/SemaInit.cpp

6919–6920

We should not be producing warnings on address-of-label cases for LK_StmtExprResult, similarly to what we do here for DeclRefExprs. If we fixed that (which I think is just a bug I introduced while refactoring this code), we'd no longer reject the example from the Linux kernel.

[rnk]

In D50805#1201805, @rsmith wrote:

There is no guarantee that you can use an address-of-label value from one function invocation in another invocation of the same function. GCC's documentation says it's the user's own problem to prevent inlining and cloning if the program requires an address-of-label extension to always produce the same value for multiple invocations of the function. It might make sense to suppress the warning in the case where the function is __attribute__((noinline)), though.

That's interesting. I'm pretty sure LLVM will not inline a function that has indirectbr to avoid this problem.

I think the state machine use case is real, though, something like:

void *f(void *startlabel) {
  common_work();
  goto *startlabel;
state1:
  return &&state2;
state2:
  return &&state3;
...
}

I think that, ultimately, this warning isn't worth the code complexity in clang to support it. Suppressing it under noinline doesn't solve the original statement expression issue either, and creates more unnecessary code complexity.

Are we really worried about users doing this?

void *f() {
  return &&next;
next:
  ...
}
void g() {
  void *pc = f();
  goto *pc; // cross-functional frame re-entry! =D
}

[• Quuxplusone]

I think that, ultimately, this warning isn't worth the code complexity in clang to support it.

The code complexity is at most 11 lines (the 11 lines removed by this patch), which really isn't much. But there's an interface-complexity argument to be made, for sure. If you added a new option -Wret-addr-label as suggested above (for a total patch of +2 lines), then is it accurate to say:

• if -Wret-addr-label was enabled by default, we know of at least one codebase that would pass -Wno-ret-addr-label to their build
• if -Wret-addr-label was disabled by default, we don't know of any codebases that would voluntarily enable it

And if nobody would enable it voluntarily... might as well eliminate it, right?

clang/test/Analysis/stack-addr-ps.cpp

81 ↗

(On Diff #160913)

Is it just me, or is this test case relying on lifetime extension for no good reason? Why is this not void *const x = &&label;?

[Meinersbur]

In D50805#1201956, @Quuxplusone wrote:

If you added a new option -Wret-addr-label as suggested above (for a total patch of +2 lines), then is it accurate to say:

• if -Wret-addr-label was enabled by default, we know of at least one codebase that would pass -Wno-ret-addr-label to their build
• if -Wret-addr-label was disabled by default, we don't know of any codebases that would voluntarily enable it

And if nobody would enable it voluntarily... might as well eliminate it, right?

That nobody here can name a project that would enable it, does not mean that there is none, or that projects will decide in the future to use it, or that individual developers temporarily use. Besides, it'd be enabled by -Weverything.

[lebedev.ri]

In D50805#1202578, @Meinersbur wrote:

In D50805#1201956, @Quuxplusone wrote:

If you added a new option -Wret-addr-label as suggested above (for a total patch of +2 lines), then is it accurate to say:

• if -Wret-addr-label was enabled by default, we know of at least one codebase that would pass -Wno-ret-addr-label to their build
• if -Wret-addr-label was disabled by default, we don't know of any codebases that would voluntarily enable it

And if nobody would enable it voluntarily... might as well eliminate it, right?

That nobody here can name a project that would enable it, does not mean that there is none, or that projects will decide in the future to use it, or that individual developers temporarily use. Besides, it'd be enabled by -Weverything.

+1 i strongly believe this should just be adding a new diag group, not completely dropping the diagnostic.

[rsmith]

In D50805#1201910, @rnk wrote:

I think the state machine use case is real, though, something like:

void *f(void *startlabel) {
  common_work();
  goto *startlabel;
state1:
  return &&state2;
state2:
  return &&state3;
...
}

Per GCC's documentation, this code is wrong, and __attribute__((noinline, noclone)) must be used. However, GCC's behavior on that case is extremely interesting: https://godbolt.org/g/xesYK1

Note that it produces a warning about returning the address of a label, just like we do. But it also says that f cannot be inlined because it contains a computed goto. So perhaps the GCC documentation / spec for the feature is wrong.

Suppressing it under noinline doesn't solve the original statement expression issue

True, but it's straightforward to fix the regression for the statement expression case. I don't think that affects whether we go further than that and remove the warning for the return case.

Are we really worried about users doing this?

void *f() {
  return &&next;
next:
  ...
}
void g() {
  void *pc = f();
  goto *pc; // cross-functional frame re-entry! =D
}

A little, yes. But I think this cross-function case alone would not justify an enabled-by-default warning with false-positives on the state machine case. So I think the question is, do we think the warnings on the state machine case are false positives or not? Some options I think are reasonable:

1. We explicitly document that our extension provides an additional guarantee on top of GCC's -- that the address of a label always denotes the same label for multiple invocations of the same function -- and remove this warning (or downgrade to -Wgcc-compat).
2. We keep merely inheriting the specification (such as it is) for the feature from GCC's documentation, and the warning is arguably valid because any code that triggers it is (almost certainly) wrong, even though we won't currently exploit the wrongness.

Or secret option 1b: we get the GCC folks to update their documentation to explicitly state that the presence of a computed goto in a function prevents inlining and cloning in all circumstances, and remove the warning.

I've filed gcc.gnu.org/PR86983; hopefully we'll get more clarification on what the semantics of this extension are supposed to be there.

[rnk]

Sounds good. I think, in the meantime, we all agree we can stop emitting this warning in the statement expression case. I'll upload a patch for that.

[rnk]

• keep the warning, suppress stmt expr case

[rnk]

• fix it right

[rsmith]

clang/lib/Sema/SemaInit.cpp

6927–6928

You should return false; if you don't produce a diagnostic here. If you carry on, you'll emit notes below and they'll get attached to whatever other diagnostic was produced most recently.

[rnk]

• return to avoid bad notes

[nickdesaulniers]

Thank you for this!

clang/lib/Sema/SemaInit.cpp

case LK_New:
       }
       break;
 
     case LK_Return:
     case LK_StmtExprResult:
       if (auto *DRE = dyn_cast<DeclRefExpr>(L)) {
         // We can't determine if the local variable outlives the statement
         // expression.
         if (LK == LK_StmtExprResult)
           return false;
         Diag(DiagLoc, diag::warn_ret_stack_addr_ref)
             << Entity.getType()->isReferenceType() << DRE->getDecl()
             << isa<ParmVarDecl>(DRE->getDecl()) << DiagRange;
       } else if (isa<BlockExpr>(L)) {
         Diag(DiagLoc, diag::err_ret_local_block) << DiagRange;
-      } else if (isa<AddrLabelExpr>(L)) {
+      } else if (isa<AddrLabelExpr>(L) && LK == LK_Return) {
         Diag(DiagLoc, diag::warn_ret_addr_label) << DiagRange;
       } else {
         Diag(DiagLoc, diag::warn_ret_local_temp_addr_ref)
          << Entity.getType()->isReferenceType() << DiagRange;
       }
       break;
     }
 
     for (unsigned I = 0; I != Path.size(); ++I) {
       auto Elem = Path[I];

clang/test/Sema/statements.c

 }
 
 
 void *test10() { 
 bar:
   return &&bar;  // expected-warning {{returning address of label, which is local}}
 }
 
+// PR38569: Don't warn when returning a label from a statement expression.
+void test10_logpc(void*);
+void test10a() {
+  test10_logpc(({
+    my_pc:
+      &&my_pc;
+  }));
+}
+
 // PR6034
 void test11(int bit) {
   switch (bit)
   switch (env->fpscr)  // expected-error {{use of undeclared identifier 'env'}}
   {
   }
 }