[clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[tbaeder]

Summary

Clang uses GNU-style attributes in objc code in (for example, I guess?) tests/Parser/stmt-attributes.m:

__attribute__((nomerge)) @try {
  [getTest() foo];
} @finally {
}

Once the ProhibitAttributes() call in question actually starts handling GNU-style attributes, these tests fail.

The call was introduced in c202b2809ac814bcae8553cd772ec4901fdb8441 by Richard Smith (I'll add him as a reviewer), but with a TODO comment stating that it might be incorrect.

I'm having a hard time finding any concrete information on whether GNU-style attributes on @try/@catch statements are allowed in objc, so maybe someone with more experience can comment?

Thanks,
Timm

[aaron.ballman]

Adding some more reviewers who know ObjC better than I do.

[rjmccall]

We don't have any reason to allow attributes on any of the @ statements, but there's no reason to disallow them grammatically, as long as we still diagnose them as invalid (which I assume is tested somewhere?).

[tbaeder]

Well we test them on @try in tests/Parser/stmt-attributes.m, but they are not being diagnosed as invalid. Should I instead keep the ProhibitAttributes() call and change the test to make sure they are being diagnosed?

[rjmccall]

We should never silently accept and ignore an attribute unless (1) that's allowable semantics for the attribute or (2) we have to for source compatibility. That test is specifically checking that we allow __attribute__((nomerge)) before @try statements. Are we just dropping the attribute, or are we correctly applying it to calls within the statement?

[tbaeder]

They are being applied to the @try at least (-ast-print prints them) and we do some error checking for missing call expressions in handleNoMergeAttr() in SemaStmtAttr.cpp. I don't know much about Objective C so I am not sure how to check that the attribute really has any effect in the end.

[tbaeder]

John, do you have any more comments on this?

[rjmccall]

In D97371#2596643, @tbaeder wrote:

They are being applied to the @try at least (-ast-print prints them) and we do some error checking for missing call expressions in handleNoMergeAttr() in SemaStmtAttr.cpp. I don't know much about Objective C so I am not sure how to check that the attribute really has any effect in the end.

It's just like try/catch/finally in languages like Java, where you can put arbitrary statements inside the nested blocks. The attribute should affect all child statements. Just find an existing nomerge test case that checks that IRGen is affected, put that code inside a @try { <stuff in here> } @catch(...) {}, and verify that the IRGen for that code is still affected. Maybe also verify that it works if you put it inside the @catch or @finally block.

[tbaeder]

Thank you.

I'm not looking at this test case:

void opaque(void);
void opaque2(void);
void opaque3(void);

@class C;

int main(int argc, const char * argv[]) {
  __attribute__((nomerge)) @try {
    opaque();
  } @catch(C *c) {
    opaque2();
  } @finally {
    opaque3();
  }
    return 0;
}

and compiling with -S -emit-llvm

shows:

%0 = type opaque

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @main(i32 %argc, i8** %argv) #0 {
entry:
  ; ...
  call void @opaque() #2
  ; ...

cleanup:                                          ; preds = %entry, %catch
  %cleanup.dest.saved = load i32, i32* %cleanup.dest.slot, align 4
  call void @opaque3() #2
  %finally.shouldthrow = load i1, i1* %finally.for-eh, align 1
  br i1 %finally.shouldthrow, label %finally.rethrow, label %finally.cont

catch:                                            ; No predecessors!
  ; ...
  call void @opaque2() #2
  ; ...

attributes #2 = { nomerge }

So all three function calls have the nomerge attribute.

I can't find an existing test case checking that nomerge, shall I just add this one in clang/test/CodeGenObjC in this patch?

[rjmccall]

Yes, I think that would be reasonable, thank you.

[rjmccall]

LGTM, thanks for seeing this through

[tbaeder]

Hmm, the new test seems to cause an assertion failure in llvm code generation in Windows. Is anything known about that? Is the test case wrong in some way?

[rjmccall]

Windows exceptions code-generation is quite different; I don't know whether Clang supports ObjC on Windows in general. It'd be fine if you add a -triple argument to this test.

[aaron.ballman]

In D97371#2641800, @rjmccall wrote:

Windows exceptions code-generation is quite different; I don't know whether Clang supports ObjC on Windows in general. It'd be fine if you add a -triple argument to this test.

I'd have no issue with that either.

[aaron.ballman]

LGTM!

[tbaeder]

Thanks everyone!

clang/lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
   // the token to end in a semicolon (in which case SemiError should be set),
   // or they directly 'return;' if not.
 Retry:
   tok::TokenKind Kind  = Tok.getKind();
   SourceLocation AtLoc;
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
     {
-      ProhibitAttributes(Attrs); // TODO: is it correct?
       AtLoc = ConsumeToken();  // consume @
       return ParseObjCAtStatement(AtLoc, StmtCtx);
     }
 
   case tok::code_completion:
     cutOffParsing();
     Actions.CodeCompleteOrdinaryName(getCurScope(), Sema::PCC_Statement);
     return StmtError();

clang/test/CodeGenObjC/attr-nomerge.m

+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -triple x86_64-unknown-linux -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+    opaque();
+  } @catch(...) {
+    opaque2();
+  } @finally {
+    opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}