This is an archive of the discontinued LLVM Phabricator instance.

Warn of uninitialized variables on asm goto's indirect branch
ClosedPublic

Authored by void on Dec 10 2019, 4:17 PM.

Details

Summary

Outputs from an asm goto block cannot be used on the indirect branch.
It's not supported and may result in invalid code generation.

Diff Detail

Event Timeline

void created this revision.Dec 10 2019, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 4:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
clang/lib/Analysis/UninitializedValues.cpp
590

Is this more concise with llvm::any_of?

789

Should we mark the vals MayUninitialized here, rather than below? Then I think we can remove the isAsmGoto check you added in getUninitUse?

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

I wonder if it's straight forward to make this a "maybe uninitialized" warning, instead of "is uninitialized?" Consider the inline asm:

asm volatile goto("ja %l1; mov %0, 42" : "=r"(foo) ::: bar);

Since we can't introspect the inline asm, we're not sure whether foo gets initialized by the asm or not (as the asm could transfer control flow back to the C label before any assignments to the output). Telling the user it's definitely uninitialized is technically correct in this case, but definitely incorrect for:

asm volatile goto("mov %0, 42; ja %l1;" : "=r"(foo) ::: bar);
38

Are we able to catch backwards branches from asm goto? (if so, would you mind please added that as an additional unit test).

int foo;
goto 1;
2:
return foo; // should warn?
1:
asm goto ("": "=r"(foo):::2);
return foo;
void updated this revision to Diff 233738.Dec 12 2019, 11:15 PM
void marked 3 inline comments as done.

Use "any_of" and improve tests.

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

The back end doesn't know how to generate code for a use in the indirect branches. It's really complicated and may result in code that doesn't actually work. I don't want to give off the impression that the code may work in these cases, because it would be essentially working by accident.

38

Yes, we should be able to warn about this. I added a testcase.

strange, the below test isn't warning for me with this patch applied:

$ clang -O2 foo.c -c
$ cat foo.c
int quux(void) {
  int y;
  asm volatile goto("ja %l1" : "=r"(y) ::: err);
  return y;
err:
  return y;
}
void added a comment.Dec 17 2019, 4:14 PM

strange, the below test isn't warning for me with this patch applied:

$ clang -O2 foo.c -c
$ cat foo.c
int quux(void) {
  int y;
  asm volatile goto("ja %l1" : "=r"(y) ::: err);
  return y;
err:
  return y;
}

Use -Wuninitialized.

void updated this revision to Diff 234657.Dec 18 2019, 8:56 PM

Check that the use isn't in the fallthrough block.

Use -Wuninitialized.

D'oh!

clang/lib/Analysis/UninitializedValues.cpp
789

Bumping comment.

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

I think if you left out indirect, it would be clearer what this test is testing.

clang/lib/Analysis/UninitializedValues.cpp
789

Ah, I see this was added in https://reviews.llvm.org/D69876.

nickdesaulniers added inline comments.Jan 6 2020, 9:31 AM
clang/lib/Analysis/UninitializedValues.cpp
798

Can you walk me through the logic of this function?

I would assume for changes to asm goto, the above early return would have been removed? Otherwise we seem to be changing the logic of the non-asm goto case?

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

bump

void updated this revision to Diff 244844.Feb 15 2020, 2:14 PM

Use "auto" and change labels to clarify test.

void marked 2 inline comments as done.Feb 15 2020, 2:15 PM
void updated this revision to Diff 245063.Feb 17 2020, 6:07 PM

Variable can't be 'const'

void marked an inline comment as done.Feb 24 2020, 7:03 PM
void added inline comments.
clang/lib/Analysis/UninitializedValues.cpp
798

Sure. If the variable hasn't been initialized by the time it gets to the asm goto, then we mark it as "potentially uninitialized" so that it's caught on the indirect branch. Uses on the default/fallthrough path should resolve to "initialized".

I'll add a comment.

void updated this revision to Diff 246363.Feb 24 2020, 7:07 PM

Explain what's going on in VisitGCCAsmStmt better.

void updated this revision to Diff 246849.Feb 26 2020, 3:35 PM
void retitled this revision from Emit a warning if a variable is uninitialized in indirect ASM goto destination. to Warn of uninitialized variables on asm goto's indirect branch.
void edited the summary of this revision. (Show Details)

Rebasing.

void added a comment.Mar 10 2020, 5:28 AM

Friendly ping. :-)

nickdesaulniers accepted this revision.Mar 10 2020, 9:17 AM
This revision is now accepted and ready to land.Mar 10 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.