Page MenuHomePhabricator

[RFC prototype] Implementation of asm-goto support in clang
Needs ReviewPublic

Authored by jyu2 on Jan 10 2019, 5:25 PM.

Details

Summary

This patch accompanies the RFC posted here:
http://lists.llvm.org/pipermail/llvm-dev/2018-October/127239.html

This is the clang side. The llvm version is in D53765

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jyu2 added inline comments.Mon, Feb 4, 11:13 AM
include/clang/AST/Stmt.h
2858

should I change to getOperandExpr?
Thanks.

lib/Analysis/CFG.cpp
1460

I am totally wrong. Changed

1467

I did this during in VisitGCCAsmStmt by not call addSuccessor when we need backpatch the the labels.

lib/Parse/ParseStmtAsm.cpp
839

Yes. Changed.
Thanks.

efriedma added inline comments.Mon, Feb 4, 1:12 PM
lib/Sema/JumpDiagnostics.cpp
350

This doesn't look right; I think we need to add it to IndirectJumps instead. This probably impacts a testcase like the following:

struct S { ~S(); };
int f() {
  {
    S s;
    asm goto(""::::BAR);
    return 1;
  }
BAR:
  return 0;
}

(gcc currently accepts this and skips running the destructor, but I'm pretty sure that's a bug.)

jyu2 marked 2 inline comments as done.Mon, Feb 4, 2:08 PM
jyu2 added inline comments.
lib/Sema/JumpDiagnostics.cpp
350

Hi Eli,
I see both g++ and clang++ with my change call ~S. Am I missing something? Thanks.

1>clang++ j.cpp
1>./a.out
~S()
1>g++ j.cpp
1>./a.out
~S()

Here is the test case:
1>cat j.cpp
extern "C" int printf (const char *,...);
struct S { ~S() {printf("~S()\n");}; };
int f() {

{
  S s;
  asm goto(""::::BAR);
  return 1;
}

BAR:

return 0;

}

int main()
{

f();

}

rsmith added inline comments.Mon, Feb 4, 2:24 PM
lib/AST/Stmt.cpp
470–475

Please assert that you don't have both outputs and labels here. (If you did, you would assign them the same slots within Exprs.)

You also seem to be missing Sema enforcement of the rule that you cannot have both outputs and labels. (If you want to actually support that as an intentional extension to the GCC functionality, you should change the implementation of GCCAsmStmt to use different slots for outputs and labels, add some tests, and add a -Wgcc-compat warning for that case. Seems better to just add an error for it for now.)

lib/Analysis/CFG.cpp
1444–1457

Please factor out this duplicated code into a local lambda rather than copy-pasting it.

1455

This check is redundant; there will be no labels if it's not an asm goto so you can just perform the below loop unconditionally.

3138–3140

Do we not need the handling for the other labels if we hit this condition for one label?

lib/Sema/SemaStmtAsm.cpp
656

No space after pair, please.

659–662

Looping over all three lists of expressions here is effectively hardcoding an implementation detail of GCCAsmStmt into this code, violating its encapsulation. Instead, please write three loops (iterating over the inputs, outputs, and labels), and remove getOperandExpr entirely.

660

Checking whether the name is empty is redundant: we should never create an IdentifierInfo representing an empty string.

efriedma added inline comments.Mon, Feb 4, 3:48 PM
lib/Sema/JumpDiagnostics.cpp
350

Oh, sorry, I wasn't paying attention to the actual contents of the asm. If you modify the asm goto slightly, to asm goto("jmp %0"::::BAR);, you'll see that the destructor doesn't run, at least with gcc. (This is different from the way goto BAR; works.)

jyu2 updated this revision to Diff 185206.Mon, Feb 4, 6:01 PM
jyu2 marked 8 inline comments as done.

More review comments addressed.

jyu2 added inline comments.Mon, Feb 4, 6:02 PM
lib/AST/Stmt.cpp
470–475

This is enforcement during the parer.

for example:
a.c:12:23: error: expected ':'
asm goto ("decl %0;" :"a": "m"(cond) : "memory" );

Do you think this is enough for now?
Thanks.
Jennifer

lib/Analysis/CFG.cpp
1444–1457

Thanks. Changed.

1455

Okay removed. Thanks.

3138–3140

It is handled in backpatch processing which go though each label and build CFG.

lib/Sema/JumpDiagnostics.cpp
350

Thanks. I see, the cleanup code is not generated for asm goto.

For clang with our current implementation, llvm emit error for this. But the error message isn't clear.

You are right, we should emit error in clang. I will look into tomorrow.

Thanks.

lib/Sema/SemaStmtAsm.cpp
656

Sorry, changed.

659–662

changed.

660

Changed. Thanks.

rsmith added inline comments.Mon, Feb 4, 6:32 PM
lib/AST/Stmt.cpp
470–475

Thanks, I see it now. Please still add the assert here.

I'd also like a custom diagnostic for that parse error; it'd seem easy and useful to add an "asm goto cannot have output constraints" error.

craig.topper added inline comments.
lib/CodeGen/CGStmt.cpp
2245

I don't think we need to pass FTy here now. I made that change before @jyknight added FunctionCallee that should be able to find the type from the InlineAsm. The CallBr IRBuilder code should have a FunctionCallee Create function as of yesterday.

jyu2 updated this revision to Diff 185360.Tue, Feb 5, 11:33 AM

1>emit better error for use of output constraints.
2> Add assert for NumLabels and NumOutputs are both larger than 0.

jyu2 marked 2 inline comments as done.Tue, Feb 5, 11:35 AM
jyu2 added inline comments.
lib/AST/Stmt.cpp
470–475

Okay added.

lib/CodeGen/CGStmt.cpp
2245

Removed.

rsmith added inline comments.Tue, Feb 5, 4:35 PM
lib/AST/Stmt.cpp
474–475

This can be simplified a little:

assert(!(NumOutputs && NumLabels) && "asm goto cannot have outputs");
639

N < NumOperands - getNumLabels()

641

I'm curious why we're checking this here, when all the other validation for the modifier letter happens in LLVM. Does this check need to be done differently from the others?

lib/CodeGen/CGStmt.cpp
2195

Nit: I think something like "asm.fallthrough" or "asm.cont" would be a more obvious name for the fall-through block.

lib/Sema/SemaStmtAsm.cpp
659–671

This is still exposing / relying on an implementation detail of GCCAsmStmt. Please replace getIdentifier with getOuptutIdentifier / getInputIdentifier / getLabelIdentifier, adjust the indexing to match, and remove GCCAsmStmt::getIdentifier. Or alternatively, iterate over Names here rather than walking the parts of the GCCAsmStmt.

jyu2 updated this revision to Diff 185477.Tue, Feb 5, 7:43 PM
jyu2 marked 4 inline comments as done.

More change from review.

Add scope checking for asm-goto.

jyu2 added inline comments.Tue, Feb 5, 7:45 PM
lib/AST/Stmt.cpp
474–475

Thanks. Yes, this is better than me. Changed

641

This is to verify following in spec:

To reference a label in the assembler template, prefix it with ‘%l’ (lowercase ‘L’) followed by its (zero-based) position in GotoLabels plus the number of input operands. For example, if the asm has three inputs and references two labels, refer to the first label as ‘%l3’ and the second as ‘%l4’).

Was request from Eli.

lib/CodeGen/CGStmt.cpp
2195

Changed.

lib/Sema/SemaStmtAsm.cpp
659–671

Changed. Like this!!!

jyu2 updated this revision to Diff 185489.Tue, Feb 5, 9:43 PM

small fix.

kees added a subscriber: kees.Wed, Feb 6, 5:54 AM

Not sure if this is the fault of the LLVM half or the Clang half, but I'm seeing mis-compilations in the current patches (llvm ca1e713fdd4fab5273b36ba6f292a844fca4cb2d with D53765.185490 and clang 01879634f01bdbfac4636ebe03b68e85b20cd664 with D56571.185489). My earlier builds were okay (llvm b1650507d25d28a03f30626843b7b133796597b4 with D53765.183738 and clang 61738985ebe78eeff6cfae7f97543d3456bac25a with D56571.181973).

I narrowed the failure down to the kernel's move_addr_to_user() function:

static int move_addr_to_user(struct sockaddr_storage *kaddr, int klen,
                             void __user *uaddr, int __user *ulen)
{
        int err;
        int len;

        BUG_ON(klen > sizeof(struct sockaddr_storage));
        err = get_user(len, ulen);
        if (err)
                return err;
        if (len > klen)
                len = klen;
        if (len < 0)
                return -EINVAL;
        if (len) {
                if (audit_sockaddr(klen, kaddr))
                        return -ENOMEM;
                if (copy_to_user(uaddr, kaddr, len))
                        return -EFAULT;
        }
        return __put_user(klen, ulen);
}

Working build produces:

ffffffff81c217a0 <move_addr_to_user>:
ffffffff81c217a0:       e8 5b fc 3d 00          callq  ffffffff82001400 <__fentry__>
ffffffff81c217a5:       81 fe 81 00 00 00       cmp    $0x81,%esi
ffffffff81c217ab:       0f 83 c8 00 00 00       jae    ffffffff81c21879 <move_addr_to_user+0xd9>
ffffffff81c217b1:       55                      push   %rbp
ffffffff81c217b2:       48 89 e5                mov    %rsp,%rbp
ffffffff81c217b5:       41 57                   push   %r15
ffffffff81c217b7:       41 56                   push   %r14
ffffffff81c217b9:       41 55                   push   %r13
ffffffff81c217bb:       41 54                   push   %r12
ffffffff81c217bd:       53                      push   %rbx
ffffffff81c217be:       49 89 ce                mov    %rcx,%r14
ffffffff81c217c1:       49 89 d7                mov    %rdx,%r15
ffffffff81c217c4:       41 89 f5                mov    %esi,%r13d
ffffffff81c217c7:       49 89 fc                mov    %rdi,%r12
ffffffff81c217ca:       48 c7 c7 36 35 88 82    mov    $0xffffffff82883536,%rdi
ffffffff81c217d1:       be da 00 00 00          mov    $0xda,%esi
ffffffff81c217d6:       e8 25 8b 5c ff          callq  ffffffff811ea300 <__might_fault>
ffffffff81c217db:       4c 89 f0                mov    %r14,%rax
ffffffff81c217de:       e8 2d f6 2f 00          callq  ffffffff81f20e10 <__get_user_4>
ffffffff81c217e3:       85 c0                   test   %eax,%eax
ffffffff81c217e5:       75 68                   jne    ffffffff81c2184f <move_addr_to_user+0xaf>
ffffffff81c217e7:       48 89 d3                mov    %rdx,%rbx
ffffffff81c217ea:       44 39 eb                cmp    %r13d,%ebx
ffffffff81c217ed:       41 0f 4f dd             cmovg  %r13d,%ebx
ffffffff81c217f1:       85 db                   test   %ebx,%ebx
ffffffff81c217f3:       78 65                   js     ffffffff81c2185a <move_addr_to_user+0xba>
ffffffff81c217f5:       74 48                   je     ffffffff81c2183f <move_addr_to_user+0x9f>
ffffffff81c217f7:       65 48 8b 04 25 00 4e    mov    %gs:0x14e00,%rax
ffffffff81c217fe:       01 00 
ffffffff81c21800:       48 8b 80 38 07 00 00    mov    0x738(%rax),%rax
ffffffff81c21807:       48 85 c0                test   %rax,%rax
ffffffff81c2180a:       74 05                   je     ffffffff81c21811 <move_addr_to_user+0x71>
ffffffff81c2180c:       83 38 00                cmpl   $0x0,(%rax)
ffffffff81c2180f:       74 50                   je     ffffffff81c21861 <move_addr_to_user+0xc1>
ffffffff81c21811:       48 63 db                movslq %ebx,%rbx
ffffffff81c21814:       4c 89 e7                mov    %r12,%rdi
ffffffff81c21817:       48 89 de                mov    %rbx,%rsi
ffffffff81c2181a:       ba 01 00 00 00          mov    $0x1,%edx
ffffffff81c2181f:       e8 3c 75 5f ff          callq  ffffffff81218d60 <__check_object_size>
ffffffff81c21824:       4c 89 ff                mov    %r15,%rdi
ffffffff81c21827:       4c 89 e6                mov    %r12,%rsi
ffffffff81c2182a:       48 89 da                mov    %rbx,%rdx
ffffffff81c2182d:       e8 ae 84 99 ff          callq  ffffffff815b9ce0 <_copy_to_user>
ffffffff81c21832:       48 89 c1                mov    %rax,%rcx
ffffffff81c21835:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c2183a:       48 85 c9                test   %rcx,%rcx
ffffffff81c2183d:       75 10                   jne    ffffffff81c2184f <move_addr_to_user+0xaf>
ffffffff81c2183f:       90                      nop
ffffffff81c21840:       90                      nop
ffffffff81c21841:       90                      nop
ffffffff81c21842:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c21847:       45 89 2e                mov    %r13d,(%r14)
ffffffff81c2184a:       31 c0                   xor    %eax,%eax
ffffffff81c2184c:       90                      nop
ffffffff81c2184d:       90                      nop
ffffffff81c2184e:       90                      nop
ffffffff81c2184f:       5b                      pop    %rbx
ffffffff81c21850:       41 5c                   pop    %r12
ffffffff81c21852:       41 5d                   pop    %r13
ffffffff81c21854:       41 5e                   pop    %r14
ffffffff81c21856:       41 5f                   pop    %r15
ffffffff81c21858:       5d                      pop    %rbp
ffffffff81c21859:       c3                      retq   
ffffffff81c2185a:       b8 ea ff ff ff          mov    $0xffffffea,%eax
ffffffff81c2185f:       eb ee                   jmp    ffffffff81c2184f <move_addr_to_user+0xaf>
ffffffff81c21861:       44 89 ef                mov    %r13d,%edi
ffffffff81c21864:       4c 89 e6                mov    %r12,%rsi
ffffffff81c21867:       e8 f4 1c 52 ff          callq  ffffffff81143560 <__audit_sockaddr>
ffffffff81c2186c:       89 c1                   mov    %eax,%ecx
ffffffff81c2186e:       b8 f4 ff ff ff          mov    $0xfffffff4,%eax
ffffffff81c21873:       85 c9                   test   %ecx,%ecx
ffffffff81c21875:       75 d8                   jne    ffffffff81c2184f <move_addr_to_user+0xaf>
ffffffff81c21877:       eb 98                   jmp    ffffffff81c21811 <move_addr_to_user+0x71>
ffffffff81c21879:       0f 0b                   ud2    
ffffffff81c2187b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)

The bad compilation produces:

ffffffff81c21700 <move_addr_to_user>:
ffffffff81c21700:       e8 fb fc 3d 00          callq  ffffffff82001400 <__fentry__>
ffffffff81c21705:       81 fe 81 00 00 00       cmp    $0x81,%esi
ffffffff81c2170b:       0f 83 cc 00 00 00       jae    ffffffff81c217dd <move_addr_to_user+0xdd>
ffffffff81c21711:       55                      push   %rbp
ffffffff81c21712:       48 89 e5                mov    %rsp,%rbp
ffffffff81c21715:       41 57                   push   %r15
ffffffff81c21717:       41 56                   push   %r14
ffffffff81c21719:       41 55                   push   %r13
ffffffff81c2171b:       41 54                   push   %r12
ffffffff81c2171d:       53                      push   %rbx
ffffffff81c2171e:       49 89 ce                mov    %rcx,%r14
ffffffff81c21721:       49 89 d7                mov    %rdx,%r15
ffffffff81c21724:       41 89 f5                mov    %esi,%r13d
ffffffff81c21727:       49 89 fc                mov    %rdi,%r12
ffffffff81c2172a:       48 c7 c7 36 35 88 82    mov    $0xffffffff82883536,%rdi
ffffffff81c21731:       be da 00 00 00          mov    $0xda,%esi
ffffffff81c21736:       e8 a5 8b 5c ff          callq  ffffffff811ea2e0 <__might_fault>
ffffffff81c2173b:       4c 89 f0                mov    %r14,%rax
ffffffff81c2173e:       e8 bd f5 2f 00          callq  ffffffff81f20d00 <__get_user_4>
ffffffff81c21743:       85 c0                   test   %eax,%eax
ffffffff81c21745:       0f 85 87 00 00 00       jne    ffffffff81c217d2 <move_addr_to_user+0xd2>
ffffffff81c2174b:       48 89 d3                mov    %rdx,%rbx
ffffffff81c2174e:       44 39 eb                cmp    %r13d,%ebx
ffffffff81c21751:       41 0f 4f dd             cmovg  %r13d,%ebx
ffffffff81c21755:       85 db                   test   %ebx,%ebx
ffffffff81c21757:       78 57                   js     ffffffff81c217b0 <move_addr_to_user+0xb0>
ffffffff81c21759:       74 48                   je     ffffffff81c217a3 <move_addr_to_user+0xa3>
ffffffff81c2175b:       65 48 8b 04 25 00 4e    mov    %gs:0x14e00,%rax
ffffffff81c21762:       01 00 
ffffffff81c21764:       48 8b 80 38 07 00 00    mov    0x738(%rax),%rax
ffffffff81c2176b:       48 85 c0                test   %rax,%rax
ffffffff81c2176e:       74 05                   je     ffffffff81c21775 <move_addr_to_user+0x75>
ffffffff81c21770:       83 38 00                cmpl   $0x0,(%rax)
ffffffff81c21773:       74 42                   je     ffffffff81c217b7 <move_addr_to_user+0xb7>
ffffffff81c21775:       48 63 db                movslq %ebx,%rbx
ffffffff81c21778:       4c 89 e7                mov    %r12,%rdi
ffffffff81c2177b:       48 89 de                mov    %rbx,%rsi
ffffffff81c2177e:       ba 01 00 00 00          mov    $0x1,%edx
ffffffff81c21783:       e8 b8 75 5f ff          callq  ffffffff81218d40 <__check_object_size>
ffffffff81c21788:       4c 89 ff                mov    %r15,%rdi
ffffffff81c2178b:       4c 89 e6                mov    %r12,%rsi
ffffffff81c2178e:       48 89 da                mov    %rbx,%rdx
ffffffff81c21791:       e8 4a 85 99 ff          callq  ffffffff815b9ce0 <_copy_to_user>
ffffffff81c21796:       48 89 c1                mov    %rax,%rcx
ffffffff81c21799:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c2179e:       48 85 c9                test   %rcx,%rcx
ffffffff81c217a1:       75 2f                   jne    ffffffff81c217d2 <move_addr_to_user+0xd2>
ffffffff81c217a3:       90                      nop
ffffffff81c217a4:       90                      nop
ffffffff81c217a5:       90                      nop
ffffffff81c217a6:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c217ab:       45 89 2e                mov    %r13d,(%r14)
ffffffff81c217ae:       31 c0                   xor    %eax,%eax
ffffffff81c217b0:       b8 ea ff ff ff          mov    $0xffffffea,%eax
ffffffff81c217b5:       eb 1b                   jmp    ffffffff81c217d2 <move_addr_to_user+0xd2>
ffffffff81c217b7:       44 89 ef                mov    %r13d,%edi
ffffffff81c217ba:       4c 89 e6                mov    %r12,%rsi
ffffffff81c217bd:       e8 9e 1d 52 ff          callq  ffffffff81143560 <__audit_sockaddr>
ffffffff81c217c2:       89 c1                   mov    %eax,%ecx
ffffffff81c217c4:       b8 f4 ff ff ff          mov    $0xfffffff4,%eax
ffffffff81c217c9:       85 c9                   test   %ecx,%ecx
ffffffff81c217cb:       75 05                   jne    ffffffff81c217d2 <move_addr_to_user+0xd2>
ffffffff81c217cd:       eb a6                   jmp    ffffffff81c21775 <move_addr_to_user+0x75>
ffffffff81c217cf:       90                      nop
ffffffff81c217d0:       90                      nop
ffffffff81c217d1:       90                      nop
ffffffff81c217d2:       5b                      pop    %rbx
ffffffff81c217d3:       41 5c                   pop    %r12
ffffffff81c217d5:       41 5d                   pop    %r13
ffffffff81c217d7:       41 5e                   pop    %r14
ffffffff81c217d9:       41 5f                   pop    %r15
ffffffff81c217db:       5d                      pop    %rbp
ffffffff81c217dc:       c3                      retq   
ffffffff81c217dd:       0f 0b                   ud2    
ffffffff81c217df:       90                      nop

The bad sequences starts at the je following js (at ffffffff81c217f5 in the good build and at ffffffff81c21759 in bad build). In the good build, the jump leads to:

ffffffff81c2183f:       90                      nop
ffffffff81c21840:       90                      nop
ffffffff81c21841:       90                      nop
ffffffff81c21842:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c21847:       45 89 2e                mov    %r13d,(%r14)
ffffffff81c2184a:       31 c0                   xor    %eax,%eax
ffffffff81c2184c:       90                      nop
ffffffff81c2184d:       90                      nop
ffffffff81c2184e:       90                      nop
ffffffff81c2184f:       5b                      pop    %rbx
ffffffff81c21850:       41 5c                   pop    %r12
...

In the bad build, the mov, mov, xor block does not fall through to function exit, instead it overwrites %eax before jumping to function exit:

ffffffff81c217a3:       90                      nop
ffffffff81c217a4:       90                      nop
ffffffff81c217a5:       90                      nop
ffffffff81c217a6:       b8 f2 ff ff ff          mov    $0xfffffff2,%eax
ffffffff81c217ab:       45 89 2e                mov    %r13d,(%r14)
ffffffff81c217ae:       31 c0                   xor    %eax,%eax
ffffffff81c217b0:       b8 ea ff ff ff          mov    $0xffffffea,%eax
ffffffff81c217b5:       eb 1b                   jmp    ffffffff81c217d2 <move_addr_to_user+0xd2>
...
ffffffff81c217d2:       5b                      pop    %rbx
ffffffff81c217d3:       41 5c                   pop    %r12
...

I've not been able to narrow this down further -- most changes to the C code cause the behavior to vanish, so I assume some optimization pass or something is moving things around to avoid the bug.

kees added a comment.EditedWed, Feb 6, 7:42 AM

I reduced the C code to this:

volatile int mystery = 0;

static int noinline demo(int klen)
{
        int err;
        int len;

        err = mystery * klen;
        if (err)
                return err;
        if (len > klen)
                len = klen;
        if (len < 0)
                return -EINVAL;
        if (len)
                return -ENOMEM;
        return __put_user(klen, (int __user *)NULL);
}

Something in the compilation of the asm-goto in__put_user() broke here.

rsmith added inline comments.Wed, Feb 6, 2:03 PM
lib/AST/Stmt.cpp
641

Sorry, I think my question was unclear. I understand why you need to do this check somewhere. My question is why this check needs to be *here*, given that all the other checks for the modifier letter are performed elsewhere. Splitting the checks up between multiple locations makes the system harder to maintain.

As it happens, there's a FIXME for this here: http://llvm.org/doxygen/AsmPrinterInlineAsm_8cpp_source.html#l00436 -- and a test case like void f(void *a) { asm("mov %l0, %%eax" :: "r"(a)); } currently causes LLVM to assert in a function that's supposed to be validating these modifier letters and producing a clean error if they're bad. So I think that's an LLVM bug, and fixing that LLVM bug would enforce the property we need here (that %lN only names labels).

[It's not clear to me whether we should be enforcing that %lN only names label operands, which would require the check to be in Clang rather than LLVM; GCC doesn't do that, and for instance accepts

void f() { asm goto("jmp %l0" :: "i"(&&foo)::foo); foo:; }

(at least for x86_64), though this construct doesn't seem hugely useful given that the input operand would need to be a literal address-of-label expression and you'd need to name the target label as a label operand anyway.]

kees added a comment.Wed, Feb 6, 11:15 PM

Not sure if this is the fault of the LLVM half or the Clang half, but I'm seeing mis-compilations in the current patches (llvm ca1e713fdd4fab5273b36ba6f292a844fca4cb2d with D53765.185490 and clang 01879634f01bdbfac4636ebe03b68e85b20cd664 with D56571.185489). My earlier builds were okay (llvm b1650507d25d28a03f30626843b7b133796597b4 with D53765.183738 and clang 61738985ebe78eeff6cfae7f97543d3456bac25a with D56571.181973).

This appears to be fixed with latest llvm, clang, and D53765.185703.

jyu2 marked an inline comment as done.Thu, Feb 7, 9:25 AM
jyu2 added inline comments.
lib/AST/Stmt.cpp
641

Hi Richard,

Thanks for the detail explanation.
I agree with you. Our compiler FE don't emit error for this also.

I'd happy to remove this, If we all agree.

Hi Eli,
what do you think?

Thanks.
Jennifer

efriedma added inline comments.Thu, Feb 7, 12:12 PM
lib/AST/Stmt.cpp
641

Yes, that's okay.

lib/Analysis/CFG.cpp
3130

Did you ever look at this?

craig.topper added inline comments.Thu, Feb 7, 3:48 PM
lib/AST/Stmt.cpp
641

In the llvm asm-goto patch, I removed the FIXME from AsmPrinterInlineAsm.cpp and set the Error flag if the operand is not an MBB or BlockAddress. This prevents tripping an assert on Richard's case. The error message could probably be better, but at leasts it not a crash on release builds now.

jyu2 updated this revision to Diff 185911.Thu, Feb 7, 9:41 PM
jyu2 marked an inline comment as done.

Remove check for %lN
Fix missing successor of asm goto in CFG build.

lib/Analysis/CFG.cpp
3130

Sorry, I missed this.

Yes. add_successor has to be true, since asm goto is kind of contional goto. I changed.

Thanks.

test/CodeGen/asm.c
276

I don't understand why, but this particular test case I cannot compile standalone without -emit-llvm.

int t32(int cond) {
  asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::label_true, loop);
  return 0;
loop:
  return 1;
label_true:
  return 2;
}
$ clang -target x86_64-linux-gnu -emit-llvm -c test.c
$ clang -target x86_64-linux-gnu -c test.c 
test.c:2:12: error: expected a symbol reference
  asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::label_true, loop);
           ^
<inline asm>:1:26: note: instantiated into assembly here
        testl %edi, %edi; jne "";
                                ^
1 error generated.

The produced assembly doesn't seem to contain the jump destination. Am I doing something wrong?

nickdesaulniers marked an inline comment as done.Sun, Feb 10, 12:54 AM
nickdesaulniers added inline comments.
test/CodeGen/asm.c
276

ah, need -no-integrated-as.

jyu2 updated this revision to Diff 186356.Mon, Feb 11, 4:03 PM

I find some ambiguous error for asm-goto out of scope.
Instead call VerifyIndirectOrAsmJumps one time, call that function twice one for indirect-goto one for asm-goto.

Let me know if you see any problems.

Thanks for all the review.
Jennifer

jyu2 updated this revision to Diff 186577.Tue, Feb 12, 6:09 PM

Cleanup CFG code, remove obj-lifetime code when build CFE for asm-goto.

ethanhs removed a subscriber: ethanhs.Tue, Feb 12, 6:53 PM

I think this is in good shape.

lib/Sema/JumpDiagnostics.cpp
678

Commented-out code?

We probably don't need a diagnostic for an asm goto which doesn't have any labels. If we do, it shouldn't be here.

jyu2 updated this revision to Diff 187036.Fri, Feb 15, 10:28 AM
jyu2 marked an inline comment as done.

Review comment addressed

lib/Sema/JumpDiagnostics.cpp
678

:-(!!!!

Yes, you are right! We don't need a diagnostic for asm goto. The error will be giving during parser. I changed.

Thank you so much for your review.

rsmith added inline comments.Fri, Feb 15, 5:34 PM
lib/AST/StmtProfile.cpp
324

Don't we also need to profile the labels themselves?

lib/Analysis/CFG.cpp
1472

It looks like we're not modeling the destructors for local variables as running on these CFG edges any more. Is that intentional?

lib/Sema/JumpDiagnostics.cpp
810–814

isa<GCCAsmStmt>(Jump)

857–862

Use isa here too.

lib/Sema/SemaStmtAsm.cpp
656–657

MyItemType and MyList aren't useful names for a reader of this code. Please use a better name. Maybe NamedOperand and NamedOperandList?

jyu2 updated this revision to Diff 187124.Fri, Feb 15, 7:26 PM
jyu2 marked 5 inline comments as done.

Review comments addressed.

jyu2 added inline comments.Fri, Feb 15, 7:27 PM
lib/AST/StmtProfile.cpp
324

How this can be lost? :-(

Changed.

lib/Analysis/CFG.cpp
1472

Yes. That it is my intention. Since clean up code are not generated for asm-goto.

lib/Sema/JumpDiagnostics.cpp
810–814

Yes!! changed.

857–862

changed

lib/Sema/SemaStmtAsm.cpp
656–657

Changed.

DamianT removed a subscriber: DamianT.
DamianT added a subscriber: DamianT.
DamianT removed a subscriber: DamianT.