This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jyu2 on Jan 10 2019, 5:25 PM.
Tokens
"Like" token, awarded by xianjun.

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 marked an inline comment as done.Jan 15 2019, 9:57 PM

Change error message.

Also of note: glib, a super popular library used across many Linux distro's will benefit from the implementation of asm goto (I will send them a patch to fix their version detection once we land in Clang): https://github.com/GNOME/glib/blob/1ba843b8a0585f20438d5617521f247071c6d94c/glib/gbitlock.c#L181

jyu2 updated this revision to Diff 184349.Jan 30 2019, 12:26 PM
jyu2 marked an inline comment as done.

Change test for BE IR change.

craig.topper commandeered this revision.Jan 30 2019, 9:27 PM
craig.topper edited reviewers, added: jyu2; removed: craig.topper.

Commandeering so I can update to match an IRBuilder interface change in the llvm patch

Pass FunctionType into the IRBuilder::CreateCallBr to avoid needing to make an opaque pointer update later

rsmith added a subscriber: rsmith.Feb 1 2019, 2:14 PM
rsmith added inline comments.
include/clang/AST/Stmt.h
997

Ugh. This cast violates strict aliasing, and even in the absence of strict aliasing won't work unless the Stmt base class is at offset 0 in T. The preceding assert is also wrong, as it's asserting that *I is an Expr, not that it's a T.

After r352925, you can use CastIterator<AddrLabelExpr> instead; that should substantially reduce the churn in this patch.

2840

This is already inside a class GCCAsmStmt; the GCC here seems unnecessary.

2858

Please give this a better name; getExpr is pretty meaningless.

lib/Analysis/CFG.cpp
1460

If this happens for every iteration of the loop, VD is used uninitialized after the loop.

1466

It is extremely suspicious to be overwriting the value of VD on each iteration of the loop like this. If you are assuming that the same value will be returned every time, please include an assert of that.

2121

This is not an appropriate function name for a function that is only called on asm goto. Given the convention here is to match the Stmt class hierarchy with the Visit functions, the branch on asm goto should be in the callee.

lib/CodeGen/CGStmt.cpp
1893

This is not a reasonable function name for a function that is specific to AsmStmts. Please also make this a (static) non-member function, since it cannot be called outside this source file.

2246–2253

No need to explicitly specify the template argument here; it's deducible.

lib/Parse/ParseStmtAsm.cpp
833–837

This should be inside the loop below.

839

while (true)

efriedma added inline comments.Feb 1 2019, 3:58 PM
lib/Analysis/CFG.cpp
3130

Passing add_successor=false seems suspect; this could probably use more test coverage.

lib/CodeGen/CGStmt.cpp
1893

Does this really need to be a template function? I think all the functions you're calling on "Result" are members of CallBase.

2197

The NumGccAsmGotoStmts variable is pointless; if names are enabled, createBasicBlock will automatically number all the blocks named "normal".

2199

Before the clobbers? I don't see the relationship; the clobbers are just a string.

Granted, it does need to be before we compute the type, which is shared code.

test/Parser/asm-goto.c
1 ↗(On Diff #184447)

What is this testing?

Forgot about protected scopes...

This patch is missing code and testcases for JumpScopeChecker. (The behavior should be roughly equivalent to what we do for indirect goto.)

jyu2 commandeered this revision.Feb 4 2019, 10:39 AM
jyu2 edited reviewers, added: craig.topper; removed: jyu2.
jyu2 added inline comments.
lib/Analysis/CFG.cpp
2121

Changed.

lib/CodeGen/CGStmt.cpp
1893

How about UpdateAsmCallInst?

1893

Yes. I am using CallBase instead template.

2197

Okay changed. Thanks.

2246–2253

Thanks. Changed.

lib/Parse/ParseStmtAsm.cpp
833–837

Yes. Change.

test/Parser/asm-goto.c
1 ↗(On Diff #184447)

Just testing asm goto can parser those input operands and labels. Not error checking.

Error checking is in Sema/asm-goto

jyu2 updated this revision to Diff 185094.Feb 4 2019, 11:03 AM

1> Add code for scope checking
2> Using CastInterator
3> CFG change to remove duplicate Block list

jyu2 marked 12 inline comments as done.Feb 4 2019, 11:13 AM
jyu2 added inline comments.
include/clang/AST/Stmt.h
997

Hi Richard,
Thanks for explain for this. And thank you so much for proving CastIterator!! I change the code to use this.

2840

Remove GCC

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.Feb 4 2019, 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.Feb 4 2019, 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.Feb 4 2019, 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
665

No space after pair, please.

668–671

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.

669

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

efriedma added inline comments.Feb 4 2019, 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.Feb 4 2019, 6:01 PM
jyu2 marked 8 inline comments as done.

More review comments addressed.

jyu2 added inline comments.Feb 4 2019, 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
665

Sorry, changed.

668–671

changed.

669

Changed. Thanks.

rsmith added inline comments.Feb 4 2019, 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.Feb 5 2019, 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.Feb 5 2019, 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.Feb 5 2019, 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
668–680

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.Feb 5 2019, 7:43 PM
jyu2 marked 4 inline comments as done.

More change from review.

Add scope checking for asm-goto.

jyu2 added inline comments.Feb 5 2019, 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
668–680

Changed. Like this!!!

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

small fix.

kees added a subscriber: kees.Feb 6 2019, 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.EditedFeb 6 2019, 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.Feb 6 2019, 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.Feb 6 2019, 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.Feb 7 2019, 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.Feb 7 2019, 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.Feb 7 2019, 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.Feb 7 2019, 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.Feb 10 2019, 12:54 AM
nickdesaulniers added inline comments.
test/CodeGen/asm.c
276

ah, need -no-integrated-as.

jyu2 updated this revision to Diff 186356.Feb 11 2019, 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.Feb 12 2019, 6:09 PM

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

ethanhs removed a subscriber: ethanhs.Feb 12 2019, 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.Feb 15 2019, 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.Feb 15 2019, 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
665–666

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.Feb 15 2019, 7:26 PM
jyu2 marked 5 inline comments as done.

Review comments addressed.

jyu2 added inline comments.Feb 15 2019, 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
665–666

Changed.

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

Should the langref be updated (specifically the section on blockaddress):

This value only has defined behavior when used as an operand to the ‘indirectbr’ instruction, or for comparisons against null.

https://reviews.llvm.org/D53765 touched the langref, but I think the blockaddress section can be cleaned up.

needs another rebase

jyu2 updated this revision to Diff 189205.Mar 4 2019, 2:20 PM

Rebased

@rsmith, @efriedma, @chandlerc, is this in good enough shape that we can commit and move to incremental fixes/improvement? Jennifer has had to rebase this a couple times which is making things hard for the folks testing this with the Linux kernel and needing to apply this patch themselves.

efriedma accepted this revision.Mar 6 2019, 3:25 PM

I'm fine merging in this state... but please wait for rsmith to respond before you merge.

This revision is now accepted and ready to land.Mar 6 2019, 3:25 PM
nickdesaulniers added a comment.EditedMar 6 2019, 3:42 PM

which is making things hard for the folks testing this with the Linux kernel and needing to apply this patch themselves.

If we can get inlining of callbr working first before landing this, then all of our CI won't go immediately red (as it would from landing this first, then getting inlining support). Otherwise, we'll have to come up with hacks in the kernel to keep the CI green (I have one for x86. The one for aarch64 quickly became infeasible).

I'm anxious for this to land, and the rebases are work indeed. But if we can just wait a little longer and get inlining support working, then land this, it will be much less painful than the other way around.

I'm focused on inlining support in https://reviews.llvm.org/D58260 (which doesn't look like much now, but I have changes locally and have been actively working on it this week).

Note that with:

  1. https://reviews.llvm.org/D60208 ("[X86] Extend boolean arguments to

inline-asm according to getBooleanType")

  1. https://reviews.llvm.org/D58260 ("[INLINER] allow inlining of

blockaddresses if sole uses are callbrs")

  1. https://reviews.llvm.org/D56571 ("[RFC prototype] Implementation of

asm-goto support in clang")

I can compile a mainline x86 defconfig Linux kernel and boot it in QEMU.

void added a subscriber: void.EditedApr 15 2019, 12:17 PM

This code:

; ModuleID = 'arch_static_branch.bc'
source_filename = "arch/x86/entry/vsyscall/vsyscall_64.c"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-grtev4-linux-gnu"

%struct.atomic_t = type { i32 }
%struct.jump_entry = type { i64, i64, i64 }
%struct.tracepoint_func = type { i8*, i8*, i32 }
%struct.static_key = type { %struct.atomic_t, %union.anon }
%union.anon = type { i64 }

@__tracepoint_emulate_vsyscall = external hidden global { i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, section "__tracepoints", align 8

; Function Attrs: alwaysinline noredzone nounwind sspstrong
define zeroext i1 @arch_static_branch(%struct.static_key* nocapture readnone %key, i1 zeroext %branch) {
entry:
  callbr void asm sideeffect "1:.byte 0x0f,0x1f,0x44,0x00,0\0A\09.pushsection __jump_table,  \22aw\22 \0A\09 .balign 8 \0A\09 .quad 1b, ${2:l}, ${0:c} + ${1:c} \0A\09.popsection \0A\09", "i,i,X,~{dirflag},~{fpsr},~{flags}"(%struct.static_key* bitcast (i32* getelementptr inbounds ({ i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, { i8*, { %struct.atomic_t, { %struct.j
ump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }* @__tracepoint_emulate_vsyscall, i64 0, i32 1, i32 0, i32 0) to %struct.static_key*), i1 false, i8* blockaddress(@arch_static_branch, %return))
          to label %asm.fallthrough [label %return]

asm.fallthrough:                                  ; preds = %entry
  call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"()
  br label %return

return:                                           ; preds = %asm.fallthrough, %entry
  %retval.0 = phi i1 [ false, %asm.fallthrough ], [ true, %entry ]
  ret i1 %retval.0
}

gives me this with the asm-goto patches:

$ clang -o /dev/null arch_static_branch.ll
warning: overriding the module target triple with x86_64-unknown-linux-gnu [-Woverride-module]
<inline asm>:4:15: error: expected a symbol reference in '.quad' directive
         .quad 1b, "", __tracepoint_emulate_vsyscall+8 + 0 
                     ^
error: cannot compile inline asm
1 warning and 1 error generated.

This code:

; ModuleID = 'arch_static_branch.bc'
source_filename = "arch/x86/entry/vsyscall/vsyscall_64.c"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-grtev4-linux-gnu"

%struct.atomic_t = type { i32 }
%struct.jump_entry = type { i64, i64, i64 }
%struct.tracepoint_func = type { i8*, i8*, i32 }
%struct.static_key = type { %struct.atomic_t, %union.anon }
%union.anon = type { i64 }

@__tracepoint_emulate_vsyscall = external hidden global { i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, section "__tracepoints", align 8

; Function Attrs: alwaysinline noredzone nounwind sspstrong
define zeroext i1 @arch_static_branch(%struct.static_key* nocapture readnone %key, i1 zeroext %branch) {
entry:
  callbr void asm sideeffect "1:.byte 0x0f,0x1f,0x44,0x00,0\0A\09.pushsection __jump_table,  \22aw\22 \0A\09 .balign 8 \0A\09 .quad 1b, ${2:l}, ${0:c} + ${1:c} \0A\09.popsection \0A\09", "i,i,X,~{dirflag},~{fpsr},~{flags}"(%struct.static_key* bitcast (i32* getelementptr inbounds ({ i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, { i8*, { %struct.atomic_t, { %struct.j
ump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }* @__tracepoint_emulate_vsyscall, i64 0, i32 1, i32 0, i32 0) to %struct.static_key*), i1 false, i8* blockaddress(@arch_static_branch, %return))
          to label %asm.fallthrough [label %return]

asm.fallthrough:                                  ; preds = %entry
  call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"()
  br label %return

return:                                           ; preds = %asm.fallthrough, %entry
  %retval.0 = phi i1 [ false, %asm.fallthrough ], [ true, %entry ]
  ret i1 %retval.0
}

gives me this with the asm-goto patches:

$ clang -o /dev/null arch_static_branch.ll
warning: overriding the module target triple with x86_64-unknown-linux-gnu [-Woverride-module]
<inline asm>:4:15: error: expected a symbol reference in '.quad' directive
         .quad 1b, "", __tracepoint_emulate_vsyscall+8 + 0 
                     ^
error: cannot compile inline asm
1 warning and 1 error generated.

I think things don't work right unless you disable the integrated assembler. I'm not sure why though.

void added a comment.Apr 15 2019, 5:21 PM

I think things don't work right unless you disable the integrated assembler. I'm not sure why though.

Using -no-integrated-as does allow it to compile, but it doesn't link (with ld.lld) if LTO is specified. Is there an equivalent flag / plugin-opt for lld?

There shouldn't be an empty string ("") in the asm output -- that should be a reference to the "l_yes" label, not the empty string. That seems very weird...

Even odder: running clang -S on the above without -fno-integrated-as emits a ".quad .Ltmp00" (note the extra "0"!), while with -fno-integrated-as, it properly refers to ".Ltmp0".

@void, @jyknight given that looks like a LLVM issue rather than a clang issue, can we file a bugzilla to track separately from this clang frontend patch?

martong resigned from this revision.Apr 23 2019, 7:03 AM

The changes in ASTImporter.cpp looks good to me!
And I have no competence about the rest of the change, thus I resign as a reviewer.

compudj added a subscriber: compudj.EditedApr 23 2019, 2:34 PM

Hi,

First, thanks for working on this. I am the author of the "rseq" system call in the Linux kernel, and the user-space code required to interact with that system call requires asm goto. I therefore look forward to getting asm goto support in clang.

I tried this patch on top of current clang on the following Linux kernel rseq selftests, and it fails to build. Hopefully this feedback can help you improve the current implementation.

The kernel rseq selftests can be found at:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/rseq

from the kernel source tree (on x86-64):

make headers_install
cd tools/testing/selftests/rseq
make CC=/path/to/clang

build output:

/home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ -shared -fPIC rseq.c -lpthread -o /home/efficios/git/linux/tools/testing/selftests/rseq/librseq.so
/home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ basic_test.c -lpthread -lrseq -o /home/efficios/git/linux/tools/testing/selftests/rseq/basic_test
/home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ basic_percpu_ops_test.c -lpthread -lrseq -o /home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test
In file included from basic_percpu_ops_test.c:12:
In file included from ./rseq.h:71:
./rseq-x86.h:90:26: error: expected a symbol reference
                "cmpq %[v], %[expect]\n\t"
                                       ^
<inline asm>:13:8: note: instantiated into assembly here
        jnz ""
              ^
In file included from basic_percpu_ops_test.c:12:
In file included from ./rseq.h:71:
./rseq-x86.h:102:3: error: expected a symbol reference
                RSEQ_ASM_DEFINE_ABORT(4, "", abort)
                ^
./rseq-x86.h:67:25: note: expanded from macro 'RSEQ_ASM_DEFINE_ABORT'
                __rseq_str(label) ":\n\t"                               \
                                      ^
<inline asm>:20:8: note: instantiated into assembly here
        jmp ""
              ^
In file included from basic_percpu_ops_test.c:12:
In file included from ./rseq.h:71:
./rseq-x86.h:148:30: error: expected a symbol reference
                "cmpq %%rbx, %[expectnot]\n\t"
                                           ^
<inline asm>:14:7: note: instantiated into assembly here
        je ""
             ^
In file included from basic_percpu_ops_test.c:12:
In file included from ./rseq.h:71:
./rseq-x86.h:164:3: error: expected a symbol reference
                RSEQ_ASM_DEFINE_ABORT(4, "", abort)
                ^
./rseq-x86.h:67:25: note: expanded from macro 'RSEQ_ASM_DEFINE_ABORT'
                __rseq_str(label) ":\n\t"                               \
                                      ^
<inline asm>:24:8: note: instantiated into assembly here
        jmp ""
              ^
4 errors generated.
Makefile:22: recipe for target '/home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test' failed
make: *** [/home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test] Error 1

@compudj email me the preprocessed output of basic_percpu_ops_test.c and I'll take a look. (Should be able to find my email via git log).

jyu2 added a comment.Apr 24 2019, 5:09 PM

@compudj email me the preprocessed output of basic_percpu_ops_test.c and I'll take a look. (Should be able to find my email via git log).

@nickdesaulniers, thanks for looking into this. It look like error come from: llvm/lib/MC/MCParser/AsmParser.cpp:1118.

Please let me know if that is clang problem.

Thanks.
Jennifer

Please let me know if that is clang problem.

It's an orthogonal issue with Clang's integrated assembler. @compudj confirmed that setting -no-integrated-as solves the issue (which is what we do throughout the kernel, except in a few places mostly by accident). I'm following up with @compudj off thread.

If we can get help w/ code review of:

Then I think we'll have everything we need to land this patch, for the Linux kernel's consumption.

xianjun added a subscriber: xianjun.
nickdesaulniers accepted this revision.May 24 2019, 11:57 AM

Ok, from the Linux kernel's perspective, I believe we have worked out all underlying issues in LLVM wrt callbr that would prevent us from using asm goto successfully. This patch now has my blessing; thanks for all the hard work that went into this large feature. Please wait for a final LGTM from @rsmith .

rsmith accepted this revision.May 24 2019, 12:45 PM
rsmith marked an inline comment as done.

Looks good with a few largely-mechanical changes.

include/clang/Basic/DiagnosticParseKinds.td
30

Nit: can_not -> cannot

lib/Parse/ParseStmtAsm.cpp
806

Nit: the Tok.isNot(tok::r_paren) check here is redundant, since isTokenStringLiteral() covers that check.

821

I think this should be:

if (!isGotoAsm && (Tok.isNot(tok::r_paren) || AteExtraColon)) {

otherwise we'll accept a spurious extra colon split from a :: at the end of a non-goto asm. Eg, I think in C++:

void f() { asm("" : : :: ); }

... would be incorrectly accepted, because we'll parse the final :: as introducing the clobbers list, consume the :: token, pass this check because the next token is now ), and never notice we didn't use the second half of the :: token.

lib/Sema/SemaStmtAsm.cpp
668–680

This is really neat, thanks!

672

Nit: explicitly spell this as std::stable_sort instead of relying on ADL to find it from the std:: in the type of NamedOperand.

kees added inline comments.May 30 2019, 11:21 AM
test/Analysis/asm-goto.cpp
1

Based on Nathan's comments in another thread, it seems like this needs to be:

// REQUIRES: x86-registered-target
// RUN: %clang_analyze_cc1 -triple x86_64 -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
test/CodeGen/asm-goto.c
1

Based on Nathan's comments in another thread, it seems like this needs to be:

// REQUIRES: x86-registered-target
// RUN: %clang_cc1 -O0 -triple x86_64 -emit-llvm  %s -o - | FileCheck %s
test/Sema/asm-goto.cpp
1

Based on Nathan's comments in another thread, it seems like this needs to be:

// REQUIRES: x86-registered-target
// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -fblocks -std=gnu99 %s -Wno-unreachable-code
kees added a comment.May 30 2019, 12:14 PM

Nick points out that "REQUIRES: x86-registered-target" is likely not needed.

Was this committed accidentally?

Today in master I see:

  • r362106: Revert "clang support gnu asm goto." Erich Keane <erich.keane@intel.com>
  • r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <maskray@google.com>
  • r362045: clang support gnu asm goto. Jennifer Yu <jennifer.yu@intel.com>

and yet this Phabricator review is still open.

It may be easier to rebase this into a monorepo checkout, then commit with git llvm push.
https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

As @kees noted, for new test files that contain x86 assembly, they need a -triple that specifies an x86 target, otherwise the implicit target is based on the host, which may not be x86. We still want to verify that non-x86 host can cross compile to x86 correctly. Sorry we did not catch this earlier in code review.

jyu2 added a comment.May 30 2019, 1:09 PM

jyu2 committed rG954ec09aed4f: clang support gnu asm goto

Was this committed accidentally?

Today in master I see:

  • r362106: Revert "clang support gnu asm goto." Erich Keane <erich.keane@intel.com>
  • r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <maskray@google.com>
  • r362045: clang support gnu asm goto. Jennifer Yu <jennifer.yu@intel.com>

and yet this Phabricator review is still open.

It may be easier to rebase this into a monorepo checkout, then commit with git llvm push.
https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

As @kees noted, for new test files that contain x86 assembly, they need a -triple that specifies an x86 target, otherwise the implicit target is based on the host, which may not be x86. We still want to verify that non-x86 host can cross compile to x86 correctly. Sorry we did not catch this earlier in code review.

Thanks Kees and Nick! I am currently work on those tests to see if I can make them more generic. All my test using x86 instruction. So that is not working for arm or arm64 target. Will update you once I am done for this.

jyu2 closed this revision.Jun 3 2019, 9:01 AM

jyu2 committed rGb8fee677bf8e: Re-check in clang support gun asm goto after fixing tests. (authored by jyu2).

There is a bug.. I took GCC’s example for asm goto and trunk emits:
https://godbolt.org/z/9EcTR9

Wrong jb instruction target..

Was this committed accidentally?

Today in master I see:

  • r362106: Revert "clang support gnu asm goto." Erich Keane <erich.keane@intel.com>
  • r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <maskray@google.com>
  • r362045: clang support gnu asm goto. Jennifer Yu <jennifer.yu@intel.com>

and yet this Phabricator review is still open.

It may be easier to rebase this into a monorepo checkout, then commit with git llvm push.
https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

As @kees noted, for new test files that contain x86 assembly, they need a -triple that specifies an x86 target, otherwise the implicit target is based on the host, which may not be x86. We still want to verify that non-x86 host can cross compile to x86 correctly. Sorry we did not catch this earlier in code review.

I think Jennifer just forgot the Differential Revision line, so this was never closed. The test failures resulted in her wanting the patch reverted until she could fix it (hence my revert)

There is a bug.. I took GCC’s example for asm goto and trunk emits:
https://godbolt.org/z/9EcTR9

Wrong jb instruction target..

Hmm... I don't see the jb instruction target name reflected in the IR, so I suspect @craig.topper should be made aware of the descrepancy (author of https://reviews.llvm.org/D53765).

There's still something weird in the backend, but things seem to generally work if you pass -fno-integrated-as to clang which the linux kernel does.

For future travelers, we're tracking 2 related bugs: