This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Handle lambda expressions within a method.
ClosedPublic

Authored by t-rasmud on May 11 2023, 11:06 AM.

Details

Summary

Handle lambda expressions defined within a method. Currently, such lambda expressions are visited as independent units. This causes the UnsafeBufferUsage analysis to crash if the lambda expression operates on variables defined outside it. This is because the analysis expects to see the VarDecl before seeing a DRE, which doesn't hold true for lambdas.

To handle this, we are proposing to visit such expressions while visiting the outer method, instead of visiting it independently. This patch implements this proposal.

Diff Detail

Event Timeline

malavikasamak created this revision.May 11 2023, 11:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 11:06 AM
malavikasamak added inline comments.May 11 2023, 11:10 AM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
174

Emitting this warning at this location seems confusing to me. Should this warning be emitted here? Also, in general, what is the context when this warning should be emitted.

ziqingluo-90 added inline comments.May 11 2023, 1:01 PM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
174

I think they are right. The lambda has its own p and a variables holding copies of p and a defined in outer scope.

ziqingluo-90 added inline comments.May 11 2023, 1:04 PM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
174

so maybe we could have another lambda capturing by references in order to test that warnings will be emitted at variables defined in the outer function.

ziqingluo-90 added inline comments.May 11 2023, 1:10 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
2214

I suspect that we will also skip lambdas defined in file scopes.

malavikasamak marked an inline comment as done.May 11 2023, 1:17 PM
malavikasamak added inline comments.
clang/lib/Analysis/UnsafeBufferUsage.cpp
2214

Yes, you are right. Added the isLocalClass check to handle this. Also will add a test case shortly.

malavikasamak marked an inline comment as done.May 11 2023, 5:35 PM
malavikasamak marked an inline comment as done.May 12 2023, 11:36 AM
malavikasamak added inline comments.
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
174

I agree that we should warn at variable declaration here. I was questioning whether this specific warning makes sense at this location, as it's supposed to appear at the unsafe operation IIUC.

I will debug further to see why this is being attached here in the first place.

ziqingluo-90 added inline comments.May 12 2023, 2:36 PM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
174

aha, I didn't look at the warnings carefully. I thought they were the warnings attached to VarDecls. Then you are right, they look strange.

ziqingluo-90 added inline comments.May 17 2023, 4:23 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
2234–2235

Maybe add a test showing that we will ignore implicit variables of lambda init-captures?

NoQ added a comment.May 18 2023, 6:12 PM

The solution looks great! I only have minor nitpicks.

This patch is adding new matchers, so we should make sure we do the usual stuff - document them, make them discoverable, make sure clang-query knows how to pick them up, etc.

clang/include/clang/ASTMatchers/ASTMatchers.h
1974–1979

We'll need to add documentation, update registry, etc. - the usual bureaucracy of adding a new ASTMatcher for everybody to use.

clang/lib/Analysis/UnsafeBufferUsage.cpp
2211–2212

Formatting nitpick.

2234–2235

Needs a line break.

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
142

Maybe add CHECK-NOT here, so that the test started failing when the underlying problem gets fixed, and so we gain more test coverage for free this way?

t-rasmud commandeered this revision.Jul 12 2023, 10:59 AM
t-rasmud updated this revision to Diff 539643.
t-rasmud edited reviewers, added: malavikasamak; removed: t-rasmud.

Address comments: Update registry and documentation.

t-rasmud marked 3 inline comments as done.Jul 12 2023, 11:01 AM
NoQ added inline comments.Jul 12 2023, 12:56 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
1988

This sounds a bit as if the type int[10] is matched, we should probably elaborate. WDYT of this take?

2004

Added more elaboration here as well. There are a few non-compiler-engineer folks using clang-query, I think it's helpful for them to know how compiler treats these things. Technically it's probably more of a C++ Standard formalism thing, but I wouldn't require knowing that either. The fact that structured bindings are "backed" by a copy of the original "whole" object is a really obscure and unobvious piece of formalism.

t-rasmud updated this revision to Diff 539726.Jul 12 2023, 2:13 PM
t-rasmud marked an inline comment as done.

Add generated ListASTMatchersReference.html file

t-rasmud updated this revision to Diff 539745.Jul 12 2023, 2:49 PM

Address remaining comments.

t-rasmud marked 3 inline comments as done.Jul 12 2023, 2:50 PM
t-rasmud marked an inline comment as done.
t-rasmud updated this revision to Diff 539752.Jul 12 2023, 3:05 PM

Minor: Remove Fixme that's already completed.

NoQ accepted this revision.Jul 18 2023, 1:38 PM

Alright the patch looks great! Let's remove the bits that correspond to D155134 and D155304, and then commit?

This revision is now accepted and ready to land.Jul 18 2023, 1:38 PM
t-rasmud updated this revision to Diff 541769.Jul 18 2023, 3:44 PM
t-rasmud retitled this revision from [WIP][-Wunsafe-buffer-usage] Handle lambda expressions within a method. to [-Wunsafe-buffer-usage] Handle lambda expressions within a method..Jul 18 2023, 3:47 PM
This revision was landed with ongoing or failed builds.Jul 20 2023, 10:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 10:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript