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

[nickdesaulniers]

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

[nickdesaulniers]

clang/lib/Analysis/CFG.cpp

3356

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.

[nickdesaulniers]

• VisitChildren rather than VisitStmt

[efriedma]

clang/lib/Analysis/UninitializedValues.cpp

828

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.

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

828

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]

clang/lib/Analysis/UninitializedValues.cpp

828

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?

[nickdesaulniers]

• don't bother checking if a Val is Initialized

[efriedma]

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.

[nickdesaulniers]

In D116059#3225862, @efriedma wrote:

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.

[efriedma]

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
}

[nickdesaulniers]

• add previously red -Warray-bounds test

[nickdesaulniers]

will add more tests

[nickdesaulniers]

• MOAR TESTS RAWR!!1one

[efriedma]

LGTM

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

84

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]

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

84

https://github.com/llvm/llvm-project/issues/53060

[nathanchance]

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

[jyu2]

Looks good to me. Thanks.