This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages
ClosedPublic

Authored by malavikasamak on Feb 27 2023, 12:36 PM.

Details

Summary

This patch handles unevaluated contexts to ensure no warnings are produced by the machinery for buffer access made within an unevaluated contexts. However, such accesses must be considered by a FixableGadget and produce the necessary fixits.

Diff Detail

Event Timeline

malavikasamak created this revision.Feb 27 2023, 12:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
malavikasamak requested review of this revision.Feb 27 2023, 12:36 PM

Rebased and removed spurious changes.

NoQ added inline comments.Feb 27 2023, 6:08 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
84–85

We should still visit other subexpressions when ignoreUnevaluatedContext is false.

Addressing the comment on _Generic handling and fixing the machinery to correctly track all DREs.

malavikasamak marked an inline comment as done.Mar 1 2023, 4:04 PM
malavikasamak added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
84–85

Fixed!

NoQ added a comment.Mar 7 2023, 3:28 PM

Aha, looks correct now!

We probably want some more tests to demonstrate that even though warnings aren't emitted against evaluated code, fixits still are.

clang/lib/Analysis/UnsafeBufferUsage.cpp
48

Looks like some of these spaces are tabs.

NoQ added a comment.Mar 7 2023, 3:29 PM

(these tests can also demonstrate that your fix for _Generic is correct!)

@NoQ I don't want to be annoying but I think you meant "warnings aren't emitted against UNEVALUATED code", is that right?

NoQ added a comment.Mar 8 2023, 1:08 PM

I think you meant "warnings aren't emitted against UNEVALUATED code"

💯

jkorous added inline comments.Mar 9 2023, 7:13 PM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
104–105

Do we think splitting this off as a separate test would help make the tests better?
I am somewhat worried that having many tests in a single file makes it harder to act on failures.

ziqingluo-90 accepted this revision.Mar 10 2023, 10:34 AM
ziqingluo-90 added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
1010

I like this idea: skipping unevaluated-contexts from the very beginning!

A nitpick to this change is that now we traverse the function body twice (one for fixables and one for warnings). But I suspect if the overhead (if there is any) is ever gonna be visible.

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
104–105

I agree. Other than these, this patch LGTM so I'm going to accept it.

This revision is now accepted and ready to land.Mar 10 2023, 10:34 AM
ziqingluo-90 added inline comments.Mar 21 2023, 2:43 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

Just realized that any_dre and PointerReferenceGadget can match the same node.

NoQ added inline comments.Apr 4 2023, 1:23 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

Hmm but wouldn't PointerReferenceGadget include a UPC or something?

1010

Warning gadgets can be found at arbitrary depths inside unevaluated contexts, so you can't simply whitelist a few patterns, you need to communicate with the traversal methods to properly ignore them.

So another approach to this problem would be to maintain a count of reasons why current context is unevaluated in the visitor, and have the visitor accept two sub-matchers: one is invoked every time, the other is invoked only when the count is zero. That's very ugly but we can try that if we find it useful for performance.

I also like the current approach because it maps nicely to what kinds of traversal matchers I'd like to have in clang-tidy. Namely, forEveryDescendantEvaluatedStmt() is what people actually want 90% of the time when they write forEachDescendant() and then hopefully deal with unintended consequences manually. (We still want to move these matchers to ASTMatchers.h right???)

NoQ added inline comments.Apr 4 2023, 1:31 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
176–188

While we're at it, maybe let's return to clang's existing naming convention now that the Stmt suffix makes it unambiguous?

ziqingluo-90 added inline comments.Apr 4 2023, 3:58 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

you are right, though I did encounter such a bug once. But I failed to reproduce it now. Let's not worry about this issue until it appears again.

ziqingluo-90 added inline comments.Apr 5 2023, 5:43 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

I have a reproducer now. Then I realized that it is not that any_dre and PointerReferenceGadget can race in anyOf. It is in fact FixableGadgets race in anyOf.
So it is a bug but unrelated to this patch. I will explain and fix it in a separated patch.

malavikasamak marked 2 inline comments as done.

Move tests to a different file.

malavikasamak marked 2 inline comments as done.Apr 10 2023, 12:35 PM
malavikasamak marked an inline comment as done.
malavikasamak added inline comments.Apr 10 2023, 2:22 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

@ziqingluo-90: Can you please add a link to the bug description?

ziqingluo-90 added inline comments.Apr 10 2023, 4:59 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

An example of two FixableGadgets race in anyOf is at a call statement foo(++p, p) where p is a raw pointer. The two FixableGadgets represent a pre-increment and a standalone pointer, respectively, in context of an unsafe call.

The matcher representing the context is callExpr(forEachArgumentWithParam(.... The match succeeds as long as there is at least one parameter matches the sub-matcher (i.e., pre-increment or standalone pointer). Therefore, both Gadgets match the same call statement.

A quick fix is to change anyOf to eachOf but maybe there are more efficient solutions.

malavikasamak added inline comments.Apr 13 2023, 11:32 AM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

If I recall correctly, we discussed changing this from anyOf to eachOf offline and were concerned about potential clashes among the fixits. So, it was decided to leave it alone for now and handle this as a separate patch.

But you are right, this can be a real pain point for us and we need to address this soonish. For your example, this may prevent us from generating any fixits as at least one of the of the DREs will never get claimed.

NoQ added a comment.Apr 13 2023, 12:52 PM

Code looks great now, and wow that's a lot of tests!

clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

call statement foo(++p, p)

Can we already add such test case? Regardless of whether it passes, it can help us confirm that we're not doing something terrible (eg. crashing) in such cases. At a glance, switching to eachOf() could totally crash us, so I'd rather document the potential problem in the form of a test while it's hot.

malavikasamak marked an inline comment as done.
NoQ accepted this revision.Apr 18 2023, 3:12 PM

Thanks! LGTM!

malavikasamak marked 5 inline comments as done.
malavikasamak added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
1002

So, it looks like this has already been changed to eachOf on the llvm main branch. Updating this to reflect the same.

This revision was landed with ongoing or failed builds.Apr 19 2023, 3:53 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 3:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript