SExprBuilder::translateDeclRefExpr was only looking at FunctionDecl and not also looking at ObjCMethodDecl. It should consider both because the attributes can be used on Objective-C as well.
rdar://problem/48941331
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
It seems weird how both do pretty similar things and e.g. duplicate getParamDecl.
The repro I have is 20M, creduce chokes on it, and manually reducing failed to repro... ☹️
Comment Actions
Agreed that it has getParamDecl as well, and the context would fit. It just seems weird how stuff is repeated, Iw as hoping someone would have some insight I failed at gaining through inspection of the code!
Comment Actions
It should consider both because the attributes can be used on Objective-C as well.
Well, it's not really supported that well though. There are known bugs like https://bugs.llvm.org/show_bug.cgi?id=38892, and I don't really have the time to fix that. (You're free to jump in of course.)
It seems weird how both do pretty similar things and e.g. duplicate getParamDecl.
I have no idea about ObjC at all, but I believe that ObjC methods are substantially different from ordinary functions. (Unlike C++ methods, which just have an additional implicit parameter.)
You could do something like SExprBuilder::enterCFG:
auto Parms = isa<ObjCMethodDecl>(D) ? cast<ObjCMethodDecl>(D)->parameters()
: cast<FunctionDecl>(D)->parameters();The repro I have is 20M, creduce chokes on it, and manually reducing failed to repro... ☹️
You could try declaring an ObjC method with a thread safety attribute that refers to a parameter, then somewhere else call that function. Again, I don't know ObjC, but for a C function you would do:
struct Data { Mutex mu; }
void f(struct Data *data) __attribute__ ((exclusive_locks_required(data->mu)));
void g() {
struct Data d;
Lock(&d.mu);
f(&d); // Should only work with your change.
Unlock(&d.mu);
}Then the SExprBuilder should have to replace the reference to the parameter (here data) by the value it is called with (here &d). I'll have to admit I don't find it easy to wrap my head around this logic either, so maybe I'm missing something. The tests for ObjC[++] are very brief, but you can look at the C++ tests for hints. (See test/Sema{ObjC,ObjCXX,CXX}/warn-thread-safety-analysis.{m,mm,cpp}.)
Comment Actions
I reduced the code I had to this:
struct __attribute__((capability("mutex"))) MyLock {
void lock() __attribute__((acquire_capability())) {}
void unlock() __attribute__((release_capability())) {}
};
template <class T> struct __attribute__((scoped_lockable)) Locker {
T &_l;
Locker(T &l) __attribute__((acquire_capability(l))) : _l(l) { _l.lock(); }
~Locker() __attribute__((release_capability())) { _l.unlock(); }
};
struct MyLockable {
MyLock lock;
};
@protocol MyProtocolBase;
@protocol MyProtocol <MyProtocolBase>
@end
@interface MyProtocol
@end
@interface MyProtocol ()
- (void)doIt:(struct MyLockable *)myLockable;
@end
@implementation MyProtocol
- (void)doIt:(struct MyLockable *)myLockable {
Locker<MyLock> lock(myLockable->lock);
}
@endBut now it doesn't repro... I probably missed something important :)
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 282–285 ↗ | (On Diff #191417) | Unrelated to your change, but I'm wondering if this is right. Ctx->AttrDecl is a NamedDecl, on which getCanonicalDecl() just returns *this (so we could in fact omit it here without functional change), while on FunctionDecl and ObjCMethodDecl it does something non-trivial instead. I guess I need to look into this a bit, don't let this bother you. But I think we might actually have to do the cast on both sides of the equation to get the same kind of canonical declaration. Or we make sure that Ctx->AttrDecl is already canonical. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
| 1 ↗ | (On Diff #191417) | Test is fine for me, but I would like if you could integrate it with the existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety analysis requires a bit of setup, that's why we tend to keep the tests in one file. I'll admit that the C++ tests have grown quite large, but for ObjC++ it's still very manageable. |
| 10 ↗ | (On Diff #191417) | Can we hard-code T = MyLock? We can still change it if we need it more general later. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | Sure thing! I created a header that's shared and simplified this repro a bit. I validated that the shared code was crashing before and this fixes the crash. |
Comment Actions
Adding Delesley in case he has input on design.
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 280 ↗ | (On Diff #191580) | Please don't use auto when the type is not explicitly spelled out in the initialization. |
| 282 ↗ | (On Diff #191580) | Same. |
| 283–285 ↗ | (On Diff #191580) | Also somewhat orthogonal to your changes, but... ugh, the lack of any common base between FunctionDecl and ObjcMethodDecl strikes again. I sort of wish we would introduce a CallableDecl base class that these two (and BlockDecl) could all inherit from to take care of this sort of ugly hack. |
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 283–285 ↗ | (On Diff #191580) | Seems I was wrong, because getCanonicalDecl() is actually virtual. I was confused by the different return type, apparently an overriding function doesn't need to have the same return type. So no problem here. This doesn't make our case easier: both FunctionDecl and ObjCMethodDecl inherit from both Decl and DeclContext, but independently. So we can't directly cast from DeclContext to Decl, we need to know which leaf we are. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
| 1 ↗ | (On Diff #191417) | I would still like all ObjCXX tests for -Wthread-safety in one file, because that's what we have for the other languages as well. (more or less) It's somewhat easier to see which aspects are tested then and which are not. Right now we only have tests for reported crashes, but maybe someday someone finds the time to test a bit more exhaustively. But perhaps @aaron.ballman sees this another way, in that case follow his opinion. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | On one hand, the all-in-one test files get to be unwieldy size, but on the other hand, it's nice having everything integrated into a single file for testing purposes. I'd say go with the all-in-one approach because it's consistent with how we otherwise test thread safety and there are some (albeit, minor) benefits. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | I'm not a fan of huge tests, and it's not consistent with other parts of LLVM. Specifically, this is testing for a regression, and should standalone to future edits don't refactor the test away. The existing test checks for functionality, and refactoring it is fine. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | I'm not going to insist, but the two people who do the most active work in this area in recent history just told you what they prefer. ;-) We add regression tests to the existing thread safety test files and it's worked out just fine. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | 4% of files under clang/test (and llvm/test) start with "PR". I appreciate your being gentle... and I'll gently nudge folks working on thread safety to follow the established process clang and LLVM follow ;-) Basically, this file is here to prevent regressions. It really shouldn't change, ever, and I'd like to make this explicit. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | Thanks to an IRC discussion, I now understand the point @jfb is making about this being a crashing regression test. You're right, we don't usually incorporate those into larger functionality tests. It's less about the regressions and more about the kind of regressions we're testing against. I'm fine with the approach in this patch now. |
| test/SemaObjCXX/no-crash-thread-safety-analysis.mm | ||
|---|---|---|
| 1 ↗ | (On Diff #191417) | The other test is not really a functional test, I added it in rC342600 as part of a fix for another bug that was reported as PR38896. But the underlying cause for both bugs is not an oversight, it's that ObjC[++] is just not supported, and someone was too lazy to completely turn the analysis off or disallow it. Now people try it out and unsurprisingly it doesn't work. Your test is not some weird special case, so it could become part of a more systematic functional test. In Clang it's quite usual to have one test per warning flag, and the thread safety analysis is just one flag. I guess the reason is that it makes refactoring easier: you see all the special cases in one file. That's quite nice! Have a look at some of these tests, they basically allow you to write the warning yourself. If all these special cases were spread around the code base, that would certainly make it harder to understand what a warning does. The test for C++ also contains tests for at least two bugs: PR34800, PR38640. (I just grepped this, there is probably more. You can try grep PR test/SemaCXX/warn-*.) If you fear someone is going to change the test, rename your class from MyInterface to PRXXXXX or something else which indicates a crash. Also indicate the issue it detects: that ObjC methods weren't handled in SExprBuilder::translateDeclRefExpr. Or that we test that they are handled now. |
Comment Actions
Creating a new test makes sense to me if it tests things across components. We have such tests for modules, PCH, and templates. There are also separate tests for the attribute parsing, which doesn't work terribly well in ObjC either. I would agree to making a new test for that when someone fixes PR38892. (Edit: got the bug number wrong.)
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 288 ↗ | (On Diff #191580) | Does your test run into this with an ObjCMethodDecl? I see how we run into the assignment to VD down below, but I don't understand how we could get here without annotating a method. |
Comment Actions
- Almost Never Auto.
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 280 ↗ | (On Diff #191580) | It's a DeclContext as spelled in getDeclContext, but sure I can drop the auto nonetheless. |
| 282 ↗ | (On Diff #191580) | It's a Decl (that's canonical), but same. |
| 288 ↗ | (On Diff #191580) | The problem is in the cast above: (lldb)
frame #5: clang::threadSafety::SExprBuilder::translateDeclRefExpr(this, DRE, Ctx) at ThreadSafetyCommon.cpp:280:9
277 // Function parameters require substitution and/or renaming.
278 if (const auto *PV = dyn_cast_or_null<ParmVarDecl>(VD)) {
279 const auto *FD =
-> 280 cast<FunctionDecl>(PV->getDeclContext())->getCanonicalDecl();
281 unsigned I = PV->getFunctionScopeIndex();
282
283 if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->getCanonicalDecl()) { |
Comment Actions
It's less about the regressions and more about the kind of regressions we're testing against.
But the test also verifies that no diagnostics are omitted (// expected-no-diagnostics), so it isn't just a "this doesn't crash" test. Which is why I think it's a nice seed to build more systematic tests around it. I'd generally like to test this more systematically, but that isn't made easier if we're sprinkling the test cases over the code base.
Basically, this file is here to prevent regressions.
Isn't every test about exactly that? That there was a similar bug in the past is at best informative, but not necessarily relevant. And if a functional test crashes, isn't that just as bad? I don't understand why the historical reason for a test needs to have an influence on where the test is placed. It makes much more sense to me to group tests by the code they test.
If there is a unit test for a class and you find a bug in it that makes it crash, would you write a complete new unit test? Or would you add a test case to the existing test? These files are our “poor man's unit test” for warnings.
| lib/Analysis/ThreadSafetyCommon.cpp | ||
|---|---|---|
| 282 ↗ | (On Diff #191580) | Depends on the type of Ctx->AttrDecl, some derived types of Decl have a stricter return type for getCanonicalDecl(). So I guess it boils down to what one thinks is obvious. We're erring on the “be explicit” end of the scale here. |
| 288 ↗ | (On Diff #191580) | Ok. I just thought it would be interesting to see if can we run into the if, but I can try that myself. |
Comment Actions
No, I wrote the test purely to check that the crash is gone. These tests *require* a diagnostic check, or // expected-no-diagnostics, so I put the later.
Agreed that ObjC needs to be tested more systematically, but this patch isn't doing this. It's fixing a crash, and adding a test to make sure the crash is gone.
Basically, this file is here to prevent regressions.
Isn't every test about exactly that? That there was a similar bug in the past is at best informative, but not necessarily relevant. And if a functional test crashes, isn't that just as bad? I don't understand why the historical reason for a test needs to have an influence on where the test is placed. It makes much more sense to me to group tests by the code they test.
If there is a unit test for a class and you find a bug in it that makes it crash, would you write a complete new unit test? Or would you add a test case to the existing test? These files are our “poor man's unit test” for warnings.
This is very concrete: this specific code used to cause a crash. This test has exactly this purpose, nothing more. What I'm doing is extremely normal for LLVM.
Comment Actions
The if logic does not enhance readability, but I suppose it can't be helped. Looks good to me.
Comment Actions
They only require // expected-no-diagnostics because you pass in -verify on the RUN line. That isn't needed to test the crashing regression, which may be part of what's causing confusion.
Agreed that ObjC needs to be tested more systematically, but this patch isn't doing this. It's fixing a crash, and adding a test to make sure the crash is gone.
Basically, this file is here to prevent regressions.
Isn't every test about exactly that? That there was a similar bug in the past is at best informative, but not necessarily relevant. And if a functional test crashes, isn't that just as bad? I don't understand why the historical reason for a test needs to have an influence on where the test is placed. It makes much more sense to me to group tests by the code they test.
If there is a unit test for a class and you find a bug in it that makes it crash, would you write a complete new unit test? Or would you add a test case to the existing test? These files are our “poor man's unit test” for warnings.
This is very concrete: this specific code used to cause a crash. This test has exactly this purpose, nothing more. What I'm doing is extremely normal for LLVM.
Agreed, this is a normal practice for tests verifying that a crash no longer happens.
Comment Actions
It certainly makes sense in many components, and I'm not questioning their judgement. But in the seven years prior to my involvement, no one thought it necessary to create a separate test for crashes in the thread safety analysis, and it's not because there haven't been any.
Warnings are in a different situations than the parser, optimizer passes or the backend, as they only operate read-only on the AST (or CFG). Looking at the commit log for test/SemaCXX, the majority of recent commits that fix crashes (and say so in the commit message) have been extending existing test cases instead of creating new ones. (Take for example rC337766, rC338089, rC342192, rC352047.) The thread safety analysis only looks at a single function's source-level CFG at a time. So we can reasonably consider functions as sufficiently isolated test cases and declarations as their setup.
That this doesn't work for a crash in the backend or parser is completely obvious to me. When working there, I would probably do the same and create a new file to reproduce my case in isolation. But what makes sense and is established practice in one component might not always make sense in another.
I'll leave the judgement to you, I'm just not convinced that this needs to be done.