This is an archive of the discontinued LLVM Phabricator instance.

[clang][CFG] Cleanup functions
ClosedPublic

Authored by tbaeder on Aug 8 2023, 5:12 AM.

Details

Reviewers
aaronpuchert
NoQ
steakhal
Group Reviewers
Restricted Project
Commits
rGad4a51302777: [clang][CFG] Cleanup functions
Summary

This might be a little light on the testing side.

Diff Detail

Event Timeline

tbaeder created this revision.Aug 8 2023, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 5:12 AM
tbaeder requested review of this revision.Aug 8 2023, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 5:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 551131.Aug 17 2023, 7:36 AM

For me this looks good, but I'd like @NoQ to sign off on it.

clang/lib/Analysis/CFG.cpp
1873–1874

I'm wondering if you should rename this accordingly.

1887–1889

Here I'd suggest to deduplicate Ty->getAsCXXRecordDecl(). Implicit conversion of pointers to bool is idiomatic in LLVM.

5307–5308

The unindent doesn't look right to me.

5848

Braces shouldn't be needed if you don't declare any variables.

clang/lib/Analysis/ThreadSafety.cpp
2429–2438 ↗(On Diff #551131)

Should this be part of a follow-up? (For which you might revive D152504.)

clang/test/Analysis/scopes-cfg-output.cpp
1428–1429

For our purposes, a pure declaration might be enough.

1433

Ideas for more tests (apart from imitating destructor tests):

  • A variable in a block, so that more statements run before the function returns.
  • A function with multiple return paths, each of which has to run the cleanup.
tbaeder updated this revision to Diff 552018.Aug 21 2023, 7:58 AM
tbaeder marked 4 inline comments as done.
tbaeder added inline comments.
clang/lib/Analysis/ThreadSafety.cpp
2429–2438 ↗(On Diff #551131)

Ah, yes, probably.

aaronpuchert added inline comments.Aug 21 2023, 5:59 PM
clang/test/Analysis/scopes-cfg-output.cpp
1473–1474

Interesting test! But it seems CodeGen has them swapped: compiling this snippet with clang -c -S -emit-llvm I get

define dso_local void @_Z4testv() #0 personality ptr @__gxx_personality_v0 {
  %1 = alloca %class.F, align 1
  %2 = alloca ptr, align 8
  %3 = alloca i32, align 4
  invoke void @_Z9cleanup_FP1F(ptr noundef %1)
          to label %4 unwind label %5

4:                                                ; preds = %0
  call void @_ZN1FD2Ev(ptr noundef nonnull align 1 dereferenceable(1) %1) #3
  ret void

; ...
}

So first cleanup, then destructor. This is with 17.0.0-rc2.

1480

As with the cleanup function, a definition shouldn't be necessary.

tbaeder marked an inline comment as done.Aug 22 2023, 12:17 AM
tbaeder added inline comments.
clang/test/Analysis/scopes-cfg-output.cpp
1473–1474

Interesting, I thought I checked this and used the correct order. Will re-check, thanks.

1480

Is there a way to test whether the contents of the cleanup function are being checked as well? From these tests, I only know we consider them called, but not whether we (properly) analyze their bodies in the context as well. Or is that separate from this patch?

aaronpuchert added inline comments.Aug 22 2023, 2:47 PM
clang/test/Analysis/scopes-cfg-output.cpp
1480

For now we're just adding a new element to the CFG and adapting the respective tests. The CFG is generated on a per-function basis, and the generation of one function's CFG will never look into another function's body. It might use some (declaration) properties of course, like whether it has [[noreturn]] or noexcept. Of course we should also generate a CFG for the cleanup function, but that's independent of this change.

Users of the CFG will naturally need to be taught about this new element type to “understand” it. Otherwise they should simply skip it. Since the CFG previously did not contain such elements, there should be no change for them. So we can also initially just add the element and not tell anybody about it.

We could also add understanding of the new element type to other CFG users, but you don't have to do this. If you only care about Thread Safety Analysis, then it's totally fine to handle it only there.

But let's move all changes to Thread Safety Analysis into a follow-up, so that we don't have to bother the CFG maintainers with that.

tbaeder updated this revision to Diff 552608.Aug 22 2023, 11:59 PM
tbaeder marked 2 inline comments as done.

Looks good to me, but let's wait for the CFG maintainers to approve it.

clang/lib/Analysis/ThreadSafety.cpp
2429 ↗(On Diff #552608)

Could you remove the added line?

steakhal requested changes to this revision.Aug 23 2023, 7:47 AM

This is a great improvement. When I saw that clang now supports it and e.g. the CSA operates on the CFG, I also considered adding this.
Now I don't need to do it, so many thanks!

The code looks correct to me, except that the cleanup should be before the dtor if it has any. Other than that, the code coverage also looks great, thus I would not oppose.

clang/lib/Analysis/CFG.cpp
1901–1904

This condition looks new. Is it an orthogonal improvement?

1901–1905

Shouldn't the cleanup function run first, and then the dtor of the variable?
This way the object is already "dead" even before reaching the cleanup handler.
https://godbolt.org/z/sT65boooW

2113

Can't this accept const VarDecl* instead? That way we could get rid of that const_cast above.

clang/test/Analysis/scopes-cfg-output.cpp
1473–1474

I believe this demonstrates the wrong order I also spotted earlier.

1480

Speaking of noreturn, I think it is worth demonstrating that if the cleanup function has noreturn attribute, then the dtor is not called.

This revision now requires changes to proceed.Aug 23 2023, 7:47 AM
tbaeder marked 2 inline comments as done.Aug 31 2023, 10:12 PM
tbaeder added inline comments.
clang/lib/Analysis/CFG.cpp
2113

Yes, I just didn't want to modify the existing function signature.

clang/test/Analysis/scopes-cfg-output.cpp
1473–1474

The current code generates the given CFG, i.e. the cleanup function comes first, followed by the dtor:

[B1]
  1: CFGScopeBegin(f)
  2:  (CXXConstructExpr, [B1.3], F)
  3: F f __attribute__((cleanup(f_)));
  4: CleanupFunction (f_)
  5: [B1.3].~F() (Implicit destructor)
  6: CFGScopeEnd(f)
  Preds (1): B2
  Succs (1): B0

(The comment from Aaron is from before when I had the order swapped.)

1480

Yeah, that does not work correctly right now.

tbaeder updated this revision to Diff 555251.Aug 31 2023, 10:13 PM
tbaeder marked an inline comment as done.

Not sure I have enough CFG knowledge. Do I just need to create another noreturn block for the cleanup function?

This is the CFG I get when both the cleanup function and the destructor are noreturn:

int main()
 [B4 (ENTRY)]
   Succs (1): B3

 [B1]
   1: CFGScopeEnd(f)
   Succs (1): B0

 [B2 (NORETURN)]
   1: [B3.3].~F() (Implicit destructor)
   Succs (1): B0

 [B3 (NORETURN)]
   1: CFGScopeBegin(f)
   2:  (CXXConstructExpr, [B3.3], F)
   3: F f __attribute__((cleanup(f_)));
   4: CleanupFunction (f_)
   Preds (1): B4
   Succs (1): B0

 [B0 (EXIT)]
   Preds (3): B1 B2 B3
tbaeder added inline comments.Sep 1 2023, 5:24 AM
clang/lib/Analysis/CFG.cpp
1901–1904

This check is needed because it's not implied anymore by the variable being in the list. It might be in there because it has a cleanup function.

When I added -analyzer-config cfg-lifetime=true to clang/test/Analysis/scopes-cfg-output.cpp, suddenly duplicated lifetime ends entries appeared where we have CleanupFunctions.
My output is:

void test_cleanup_functions()
 [B2 (ENTRY)]
   Succs (1): B1

 [B1]
   1: CFGScopeBegin(i)
   2: int i __attribute__((cleanup(cleanup_int)));
   3: CleanupFunction (cleanup_int)
   4: [B1.2] (Lifetime ends)
   5: [B1.2] (Lifetime ends)
   6: CFGScopeEnd(i)
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (1): B1

void test_cleanup_functions2(int m)
 [B4 (ENTRY)]
   Succs (1): B3

 [B1]
   1: 10
   2: i
   3: [B1.2] = [B1.1]
   4: return;
   5: CleanupFunction (cleanup_int)
   6: [B3.2] (Lifetime ends)
   7: [B3.2] (Lifetime ends)
   8: CFGScopeEnd(i)
   Preds (1): B3
   Succs (1): B0

 [B2]
   1: return;
   2: CleanupFunction (cleanup_int)
   3: [B3.2] (Lifetime ends)
   4: [B3.2] (Lifetime ends)
   5: CFGScopeEnd(i)
   Preds (1): B3
   Succs (1): B0

 [B3]
   1: CFGScopeBegin(i)
   2: int i __attribute__((cleanup(cleanup_int)));
   3: m
   4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
   5: 1
   6: [B3.4] == [B3.5]
   T: if [B3.6]
   Preds (1): B4
   Succs (2): B2 B1

 [B0 (EXIT)]
   Preds (2): B1 B2

void test()
 [B2 (ENTRY)]
   Succs (1): B1

 [B1]
   1: CFGScopeBegin(f)
   2:  (CXXConstructExpr, [B1.3], F)
   3: F f __attribute__((cleanup(cleanup_F)));
   4: CleanupFunction (cleanup_F)
   5: [B1.3].~F() (Implicit destructor)
   6: [B1.3] (Lifetime ends)
   7: CFGScopeEnd(f)
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (1): B1

Notice the [B3.2] (Lifetime ends) lines for example.

The order in which the Lifetime, Scope and Cleanup elements appear looks correct; my only concern is the duplicate Lifetime ends marker.

About the noreturn cleanup function, well, GCC says: It is undefined what happens if cleanup_function does not return normally. here, thus I'm not sure what to do in that case. GCC seems to optimize accordingly, but clang does not. See https://godbolt.org/z/z8s6bPPjv.

FYI Unfortunately, I don't have much experience with CFG either.

tbaeder updated this revision to Diff 555403.Sep 1 2023, 8:59 AM
tbaeder marked an inline comment as done.

@steakhal Double lifetime ends should be fixed now.

steakhal accepted this revision.Sep 1 2023, 9:29 AM

@steakhal Double lifetime ends should be fixed now.

I haven't verified, but it should be good now.

This revision is now accepted and ready to land.Sep 1 2023, 9:29 AM
aaronpuchert added inline comments.Sep 14 2023, 4:54 PM
clang/test/Analysis/scopes-cfg-output.cpp
1472

The test failure in D152504 suggests that this needs to be written as

__attribute__((cleanup(cleanup_F))) F f

Maybe something changed upstream?

tbaeder updated this revision to Diff 556836.Sep 15 2023, 2:33 AM
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/test/Analysis/scopes-cfg-output.cpp
1472

Right, not sure what changed.

This revision was automatically updated to reflect the committed changes.