This is an archive of the discontinued LLVM Phabricator instance.

[Clang][CFG] check children statements of asm goto
ClosedPublic

Authored by nickdesaulniers on Dec 20 2021, 3:36 PM.

Details

Summary

When performing CFG based analyses, don't forget to check the child
statements of an asm goto, such as the expressions used for
inputs+outputs.

Fixes: https://github.com/llvm/llvm-project/issues/51024
Fixes: https://github.com/ClangBuiltLinux/linux/issues/1439

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Dec 20 2021, 3:36 PM
nickdesaulniers created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2021, 3:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers planned changes to this revision.Dec 20 2021, 3:44 PM
nickdesaulniers added inline comments.
clang/lib/Analysis/CFG.cpp
3360

I think this should actually be return VisitChildren(G);; it produces a simpler CFG with the asm goto Node marked as the terminator directly. Will update this and the test.

  • VisitChildren rather than VisitStmt
efriedma added inline comments.Dec 20 2021, 4:21 PM
clang/lib/Analysis/UninitializedValues.cpp
826

The old and the new code here seem to be doing very different things. It isn't clear why.

Actually, I'm not sure why we're checking whether the variable is initialized. Whether the variable is initialized going into the inline asm isn't really related to whether it's initialized when we exit the inline asm. For register outputs, the output isn't even in the same register as the old value of the variable.

clang/lib/Analysis/UninitializedValues.cpp
826

I suspect we're now eagerly marking variables initialized by visiting the child nodes, which we weren't doing previously.

See also: https://reviews.llvm.org/D71314.

void added inline comments.Dec 21 2021, 12:36 PM
clang/lib/Analysis/UninitializedValues.cpp
826

I'm very confused as well. You're basically going from a strong statement - (This variable is initialized) - to a weaker statement - (This variable may be uninitialized). That doesn't make a lot of sense to me. Could you explain it better in the comment?

  • don't bother checking if a Val is Initialized
nickdesaulniers marked 2 inline comments as done.Jan 5 2022, 3:30 PM

Do we have testcases for how -Wuninitialized/-Wmaybe-uninitialized interact with asm goto? If not, can you add them?

Also, an explicit test for -Warray-bounds or whatever would be nice, although I guess the CFG tests sort of cover that.

void accepted this revision.Jan 6 2022, 11:46 AM
This revision is now accepted and ready to land.Jan 6 2022, 11:46 AM

Do we have testcases for how -Wuninitialized/-Wmaybe-uninitialized interact with asm goto?

We have tests for -Wuninitialized; clang/test/Analysis/uninit-asm-goto.cpp (D71314).

Clang doesn't implement -Wmaybe-unitialized; perhaps you're thinking of -Wsometimes-unitialized?

clang/test/Analysis/uninit-sometimes.cpp does not contain any asm goto tests (for -Wsometimes-unitialized).

If not, can you add them?

-Wsometimes-unitialized is part of the diag group for -Wunitialized. Adding another RUN line to clang/test/Analysis/uninit-asm-goto.cpp that tests -Wsometimes-unitialized rather than -Wunitialized produces the same result, so unless there's a different case you can think of, I don't see testing -Wsometimes-unitialized w/ asm goto as improving our test coverage at all.

Also, an explicit test for -Warray-bounds or whatever would be nice, although I guess the CFG tests sort of cover that.

Sure, I can add one.

nickdesaulniers planned changes to this revision.Jan 6 2022, 12:21 PM

Yes, messed up the warning flag.

Did some brief experiments on trunk; I'd like to see the following two testcases as regression tests:

int a(int z) {
    int x;
    if (z)
      asm goto ("":"=r"(x):::A1,A2);
    return 0;
    A1:
    A2:
    return x; // expected warning: conditional use of uninitialized var
}

int b() {
    int x = 0;
    asm goto ("":"=r"(x):::A1,A2);
    return 0;
    A1:
    A2:
    return x; // expected warning: use of uninitialized variable
}
  • add previously red -Warray-bounds test
This revision is now accepted and ready to land.Jan 6 2022, 12:29 PM
nickdesaulniers planned changes to this revision.Jan 6 2022, 12:30 PM

will add more tests

  • MOAR TESTS RAWR!!1one
This revision is now accepted and ready to land.Jan 6 2022, 2:06 PM
efriedma accepted this revision.Jan 6 2022, 3:59 PM

LGTM

clang/test/Analysis/uninit-asm-goto.cpp
84 ↗(On Diff #397983)

Looks like this patch fixes the false negative on test8; that's good.

It looks like there's a issue with the printed diagnostic on a bunch of these tests; we say the variable is used uninitialized "whenever its declaration is reached", which isn't right. We can probably leave that for a followup; this is clearly an improvement. But please file a bug.

nickdesaulniers marked an inline comment as done.Jan 6 2022, 4:26 PM
nickdesaulniers added inline comments.
clang/test/Analysis/uninit-asm-goto.cpp
84 ↗(On Diff #397983)
nickdesaulniers marked an inline comment as done.Jan 6 2022, 4:27 PM

The two false positive warnings in the kernel are fixed with this patch and there were no new warnings introduced.

jyknight accepted this revision.Jan 7 2022, 12:26 PM
jyu2 accepted this revision.Jan 7 2022, 1:20 PM

Looks good to me. Thanks.

This revision was landed with ongoing or failed builds.Jan 7 2022, 2:20 PM
This revision was automatically updated to reflect the committed changes.