Skip to content

[analyzer] use invalidateRegions() in VisitGCCAsmStmt #109838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 4, 2024
Merged

[analyzer] use invalidateRegions() in VisitGCCAsmStmt #109838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbec0e8
clang/csa: use invalidateRegions() in VisitGCCAsmStmt
pskrgag Sep 24, 2024
cd61671
use regex in test string
pskrgag Sep 24, 2024
d5a5c47
address review comments
pskrgag Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3810,15 +3810,19 @@ void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
assert(!isa<NonLoc>(X)); // Should be an Lval, or unknown, undef.

if (std::optional<Loc> LV = X.getAs<Loc>())
state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
state = state->invalidateRegions(*LV, A, currBldrCtx->blockCount(),
Pred->getLocationContext(),
/*CausedByPointerEscape=*/true);
}

// Do not reason about locations passed inside inline assembly.
for (const Expr *I : A->inputs()) {
SVal X = state->getSVal(I, Pred->getLocationContext());

if (std::optional<Loc> LV = X.getAs<Loc>())
state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
state = state->invalidateRegions(*LV, A, currBldrCtx->blockCount(),
Pred->getLocationContext(),
/*CausedByPointerEscape=*/true);
}

Bldr.generateNode(A, Pred, state);
Expand Down
24 changes: 18 additions & 6 deletions clang/test/Analysis/asm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@ void clang_analyzer_dump_ptr(void *);

int global;
void testRValueOutput() {
int &ref = global;
ref = 1;
int origVal = global;
__asm__("" : "=r"(((int)(global)))); // don't crash on rvalue output operand
clang_analyzer_eval(global == 1); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(ref == 1); // expected-warning{{UNKNOWN}}
int newVal = global; // Value "after" the invalidation.
clang_analyzer_eval(origVal == newVal); // expected-warning{{TRUE}} expected-warning{{FALSE}}
}

void *MyMemcpy(void *d, const void *s, const int n) {
Expand Down Expand Up @@ -40,7 +39,20 @@ void testInlineAsmMemcpyUninit(void)
{
int a[10], b[10] = {}, c;
MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
c = a[0]; // FIXME: should be warning about uninitialized value, but invalidateRegions() also
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunate, but not really sure how to avoid it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set the invalidation traint to preserve super region.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it would work, since then testInlineAsmMemcpyUninitLoop would also fail, since only the first element of the array will be invalidated.

Maybe I am missing some API to invalidate part of the region?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I think, this behavior maybe even better in terms of FP. Like maybe asm block will do smth like container_of or such.

So this might be an acceptable damage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it all boils down to what the assembly block does. I don't think there is anything fundamentally preventing us refining what gets invalidated other than actually parsing and understanding the asm block.

Invalidating everything is a good default choice; however, frequently we can do better than that.
IDK how the MyMemcpy works in your test, but it could model the invalidation similar to how the StreamChecker goes smart about it in tryToInvalidateFReadBufferByElements here.

// invalidates super region.
}

void testInlineAsmMemcpyUninitLoop(const void *src, unsigned long len)
{
int a[10], c;
unsigned long toCopy = sizeof(a) < len ? sizeof(a) : len;

MyMemcpy(a, src, toCopy);

// Use index 1, since before use of invalidateRegions in VisitGCCAsmStmt, engine bound unknown SVal only to
// first element.
c = a[1]; // no-warning
}

void testAsmWithVoidPtrArgument()
Expand All @@ -49,6 +61,6 @@ void testAsmWithVoidPtrArgument()
clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning-re {{reg_${{[0-9]+}}<int Element{SymRegion{reg_${{[0-9]+}}<void * globalVoidPtr>},0 S64b,int}>}}
clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}<void * globalVoidPtr>}}}
asm ("" : : "a"(globalVoidPtr)); // no crash
clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning {{Unknown}}
clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning {{derived_}}
clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}<void * globalVoidPtr>}}}
}
Loading