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

[jyu2]

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

[jyu2]

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.

[nickdesaulniers]

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]

test/CodeGen/asm.c

276

ah, need -no-integrated-as.

[jyu2]

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]

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

[efriedma]

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]

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]

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]

Review comments addressed.

[jyu2]

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.

[jyu2]

Rebase

[nickdesaulniers]

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.

[nickdesaulniers]

needs another rebase

[jyu2]

Rebased

[craig.topper]

@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]

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

[nickdesaulniers]

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).

[nickdesaulniers]

Note that with:

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

inline-asm according to getBooleanType")

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

blockaddresses if sole uses are callbrs")

3. 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]

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.

[craig.topper]

In D56571#1467333, @void wrote:

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]

In D56571#1467358, @craig.topper wrote:

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?

[jyknight]

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".

[craig.topper]

@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]

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]

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

[nickdesaulniers]

@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]

In D56571#1476266, @nickdesaulniers wrote:

@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

[nickdesaulniers]

In D56571#1477992, @jyu2 wrote:

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:

• https://reviews.llvm.org/D58260
• https://reviews.llvm.org/D60887
• https://reviews.llvm.org/D60224

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

[nickdesaulniers]

Also: https://reviews.llvm.org/D61560

[nickdesaulniers]

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]

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]

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]

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

[nickdesaulniers]

Was this committed accidentally?

Today in master I see:

• r362106: Revert "clang support gnu asm goto." Erich Keane <[e-mail]>
• r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <[e-mail]>
• r362045: clang support gnu asm goto. Jennifer Yu <[e-mail]>

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]

jyu2 committed rG954ec09aed4f: clang support gnu asm goto

In D56571#1523895, @nickdesaulniers wrote:

Was this committed accidentally?

Today in master I see:

• r362106: Revert "clang support gnu asm goto." Erich Keane <[e-mail]>
• r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <[e-mail]>
• r362045: clang support gnu asm goto. Jennifer Yu <[e-mail]>

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]

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

[xbolva00]

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

Wrong jb instruction target..

[erichkeane]

In D56571#1523895, @nickdesaulniers wrote:

Was this committed accidentally?

Today in master I see:

• r362106: Revert "clang support gnu asm goto." Erich Keane <[e-mail]>
• r362059: Mark CodeGen/asm-goto.c as x86 specific after r362045 Fangrui Song <[e-mail]>
• r362045: clang support gnu asm goto. Jennifer Yu <[e-mail]>

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)

In D56571#1529349, @xbolva00 wrote:

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).

[craig.topper]

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.

[nickdesaulniers]

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

• -no-integrated-as providing different results: https://github.com/ClangBuiltLinux/linux/issues/500
• IfConverter asserting on INLINEASM_BR MachineInstrs: https://github.com/ClangBuiltLinux/linux/issues/494