Page MenuHomePhabricator

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

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 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
3129

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
669

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
669

:-(!!!!

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
820–824

isa<GCCAsmStmt>(Jump)

867–872

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.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
820–824

Yes!! changed.

867–872

changed

lib/Sema/SemaStmtAsm.cpp
656–657

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–31

Nit: can_not -> cannot

lib/Parse/ParseStmtAsm.cpp
806–807

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
659–671

This is really neat, thanks!

663

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
2

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
2

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
2

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: