Page MenuHomePhabricator

Allow output constraints on "asm goto"
Needs ReviewPublic

Authored by void on Nov 5 2019, 6:18 PM.

Details

Summary

Remove the restrictions that preventing "asm goto" from returning non-void
values. The values returned by "asm goto" are only valid on the "fallthrough"
path.

Event Timeline

void created this revision.Nov 5 2019, 6:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2019, 6:18 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
void updated this revision to Diff 228594.Sat, Nov 9, 9:58 PM

Emit asm goto fallthrough early so that any value extracts will show up in the
fallthrough block and not directly after the callbr.

void updated this revision to Diff 228595.Sun, Nov 10, 1:14 AM

Adjust the ASM so that it references labels.

void edited the summary of this revision. (Show Details)Sun, Nov 10, 6:26 PM
void marked an inline comment as done.

This change is ready for review. PTAL.

I think -Wuninitialized (UninitializedValues.cpp) should be taught how to detect the use of output variables in error blocks, at least for trivial cases.

Actually, for some reason -- it looks like the warning is failing the wrong way right now, and emits an uninitialized use warning even where there shouldn't be one.

E.g. this example should be okay:

int foo(int x) {
  int y;
  asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
  return y;
err:
  return -1;
}

But now warns:

$ clang -Wall -fsyntax-only foo.c
foo.c:4:10: warning: variable 'y' is uninitialized when used here [-Wuninitialized]
  return y;
         ^
foo.c:2:8: note: initialize the variable 'y' to silence this warning
  int y;
       ^
        = 0
1 warning generated.

I'd expect a warning only if the code was modified to say "return y" in the err block.

Also, since this means we are no longer simply implementing according to GCC's documentation, I think this means we'll need a brand new section in the Clang docs for its inline-asm support.

void updated this revision to Diff 228762.Mon, Nov 11, 1:03 PM

Analyze "asm goto" for initialized variables.

An "asm goto" statement is a terminator and thus doesn't indicate variable
assignments like normal inline assembly. Handle them specially.

Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 11, 1:03 PM
void added a comment.Mon, Nov 11, 1:03 PM

I think -Wuninitialized (UninitializedValues.cpp) should be taught how to detect the use of output variables in error blocks, at least for trivial cases.

Actually, for some reason -- it looks like the warning is failing the wrong way right now, and emits an uninitialized use warning even where there shouldn't be one.

Nice catch! I updated the patch with a fix. PTAL.

void added a comment.Mon, Nov 11, 1:04 PM

Also, since this means we are no longer simply implementing according to GCC's documentation, I think this means we'll need a brand new section in the Clang docs for its inline-asm support.

Sure. Which file do we list such things in?

I think -Wuninitialized (UninitializedValues.cpp) should be taught how to detect the use of output variables in error blocks, at least for trivial cases.

Actually, for some reason -- it looks like the warning is failing the wrong way right now, and emits an uninitialized use warning even where there shouldn't be one.

Nice catch! I updated the patch with a fix. PTAL.

Please add a test for that!

void updated this revision to Diff 228775.Mon, Nov 11, 3:16 PM

the real update this time.

void added a comment.Mon, Nov 11, 3:16 PM

Nice catch! I updated the patch with a fix. PTAL.

Please add a test for that!

I did, but the "arc" program pushed the wrong change. :-( Fixed.

When are we going to use GitHub's PRs?

void updated this revision to Diff 228777.Mon, Nov 11, 3:30 PM

Add blurb about asm goto with output constraints to the "language extensions"
documentation.

void updated this revision to Diff 229021.Wed, Nov 13, 12:45 AM

Adjust label references to account for in/out constraints.

void updated this revision to Diff 229166.Wed, Nov 13, 1:01 PM

Move adjustment before error check.

void added a comment.Thu, Nov 14, 10:29 PM

Friendly ping. :-)

clang/lib/Analysis/UninitializedValues.cpp
830

GCCAsmStmt inherits from AsmStmt which has an outputs() method for returning an iterator. Please use that an a range based for loop.

875

Don't declare as as const, then you won't need this const_cast.

void updated this revision to Diff 229904.Mon, Nov 18, 12:57 PM
void marked 2 inline comments as done.

Remove a "const_cast" and use iterators for output constraints.

void added a comment.Mon, Nov 18, 12:58 PM

Changes made. PTAL

clang/lib/Analysis/UninitializedValues.cpp
831

this is still not a range based for loop.

Does:

for (const auto &O : as->outputs())
  if (const VarDecl *VD = findVar(O).getDecl())
    vals[VD] = Initialized;

not work?

void updated this revision to Diff 229921.Mon, Nov 18, 2:14 PM

Adjust loop.

void marked 2 inline comments as done.Mon, Nov 18, 2:14 PM
void added inline comments.
clang/lib/Analysis/UninitializedValues.cpp
831

Done.

void added a comment.Sun, Nov 24, 1:58 PM

Any further comments?

rnk added a subscriber: rsmith.Tue, Dec 3, 4:52 PM

Changes seem fine to me, FWIW.
+@rsmith

clang/test/Analysis/uninit-asm-goto.cpp
10

This could use a test for the case where an input is uninitialized, and where an uninitialized value is used along the error path.

void marked an inline comment as done.Tue, Dec 3, 7:01 PM
void added inline comments.
clang/test/Analysis/uninit-asm-goto.cpp
10

I have a follow-up patch that I'll upload soon that warns about uninitialized variable use on the indirect paths. I wanted to separate it to keep this patch size down. If you'd prefer I can just add it.