This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Extend bugprone-exception-escape diagnostics
AbandonedPublic

Authored by PiotrZSL on Jun 19 2023, 10:06 AM.

Details

Summary

Emit additional notes with information about thrown uncaught
exceptions at a location where they thrown or re-thrown.

Diff Detail

Event Timeline

PiotrZSL created this revision.Jun 19 2023, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 10:06 AM
Herald added a subscriber: xazax.hun. · View Herald Transcript
PiotrZSL requested review of this revision.Jun 19 2023, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 10:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
isuckatcs added inline comments.Jun 19 2023, 2:28 PM
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
84–86

I'm still not sure how I feel about this message though.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
331

This continue and the other one change the behaviour of this function. Without this some additional conditions are also checked after the else if block. Shouldn't we preserve the old behaviour?

380–383

This line makes me wonder if it's worth using a map instead of a set for ThrownExceptions. You could map the type to the location. I mean technically, that happens now too, but not with the appropriate data structure.

Also I wonder what happens if a function can throw the same type from multiple locations. E.g.:

void foo(int x) {
  if(x == 0) 
    throw 1;
  
  if(x == 1)
    throw 2;
}

Here only the last location will be preserved, so maybe mapping Type to vector<SourceLocation> would be better.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
47

What is the reason behind declaring this operator and the one below as friend?

PiotrZSL updated this revision to Diff 532783.Jun 19 2023, 9:42 PM
PiotrZSL marked 3 inline comments as done.

Review fixes.

Question would be, does this can stay +- like this and we could wait with extension this until some users for example complain, that they would like it to be extended, or this need to be done now.
Main reason for this change is, that often I run into situation when there were some warning raised for some function, but after deeper investigation it were for example due to some exceptions being thrown in some boost library.
And that hard to guess were the issue were.

clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
84–86

I also, my main reason was to show user that except listed exceptions, also some other exceptions may be thrown, that we do not know because they throw from an functions without implementation.
In ideal condition I should just raise this warning for a function that we called, that we do not have implementation, and we do not know what it throws.
I would say that this could be improved in future. Originally I raised all those notes against a main function, changed this recently to show were throws are called.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
331

No, old behavior were inefficient. All push_backs push same exception into vector, that we later remove from set. So when we execute additional ifs we still going to push same execution to vector, and as a result we going to try to remove same element from set twice. That`s not needed.

380–383

We use llvm::SmallSet, but there is no llvm::SmallMap. I wanted to preserve memory optimizations, as using some map could hit performance, after all we create many of those temporary containers.
And with map I run into some issues (it didn't like const pointer as key).

As for double throw of same type. I agree, but I don't think its worth being implemented currently.
Once user remove one exception, check will show second. Many checks work like that.
This is just to provide small hint.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
47

I just get used to do that. Usually friend enable conversion, but here we do not have constructor or derived classes, so that's not needed.
I can change this.

PiotrZSL marked an inline comment as done.Jun 19 2023, 9:42 PM
isuckatcs added inline comments.Jun 23 2023, 11:19 AM
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
380–383

I still feel like creating these {T, SourceLocation()} entries is bloated. In case of a map, you wouldn't need to create dummy SourceLocations.

We have DenseMap, which is documented like this:

DenseMap is a simple quadratically probed hash table. It excels at supporting small keys and values: [...] DenseMap is a great way to map pointers to pointers, or map other small types to each other.

Here you would map pointers to SourceLocations, which are basically ints, so they are smaller than pointers. I think it worths giving DenseMap a try.

Also note that the number of exceptions we store is very low, so even std::map<> wouldn't cause a significant performance loss. Also currently our SmallSet<> is set to 2 elements, which means if we store more than 2 elements, it will switch to std::set<> instead.

PiotrZSL added inline comments.Jun 23 2023, 11:33 AM
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
380–383

For SourceLocation{}, I can add constructors to ThrowInfo to hide it, but that's also not elegant solution.
Thing is that llvm::SmallSet here wont allocate memory for up to 2 elements, this mean no memory allocation and so on.
And because on every function we basically doing a copy of this container, or we create new one, then allocating memory with DenseMap could be potentially costly (allocate 64 elements).
I also run into compilation issues when using const Type* as a key for a DenseMap (yes I tried it before I choose this solution).
If we talk about std::map, well, that is a better option, as it's not allocate memory by default.
I can give it a try...

PiotrZSL updated this revision to Diff 534169.Jun 24 2023, 12:00 AM

Use std::map

PiotrZSL marked an inline comment as done.Jun 24 2023, 2:02 AM
PiotrZSL updated this revision to Diff 551761.Aug 19 2023, 8:33 AM

Rebase + Ping

isuckatcs added inline comments.Aug 19 2023, 11:12 AM
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
80
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
14

Is there any particular reason for taking SourceLocation by value? A const SourceLocation & would avoid an unnecessary copy.

21–22

Same question here regarding the argument type.

25–26
26

There shouldn't be any invalid location in the container. An exception is thrown by a statement, so we know where in source that happens.

324

For a moment it wasn't entirely clear for me what Fnt meant, I suggest using naming like FnType or FunctionType.

363
380–383

This introduces a side effect to this function and makes it do 2 things at the same time. Besides filtering the caught exceptions, it also collects and returns them through a reference parameter.

I don't mind the latter, but in that case I'd like to see the caught exceptions returned from the function as a part of the return statement. Also notice that if CaughtExceptions is not empty, Result is true, which makes the latter redundant. I suggest dropping the bool return type and returning the Throwables instead.

400

You are erasing something from a container on which you're iteration over. Are you sure the iterators are not invalidated, when the internal representation changes?

514

Here we rethrow something from a try block if I understand it correctly.

void foo() {
    throw 3;
}

void bar() {
    try {
        foo();
    } catch(short) {
    }
}

The best way would be to set the SourceLocation to the point, where foo() is called.
I think you would need to create a new struct called ThrowableInfo, which contains,
every information that we need to know about the Throwable e.g.: type, location, parent
context, etc. That would also be more extensible.

If there's no way to deduce this location for some reason, you can still set the location
to the try block and create messages like rethrown from try here, etc. Just don't create
invalid SourceLocations please, because besides them being incorrect, they are also
a source of bugs and crashes.

514

Also, is this branch tested? I don't see any new test case with a try block.

545
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
40

Remove this \n please, we don't have an additional new line above the ExceptionAnalyzer class either, so please keep it consistent.

PiotrZSL planned changes to this revision.Aug 19 2023, 11:39 AM
PiotrZSL marked 4 inline comments as done.

TODO: Resolve rest of review comments.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
14

SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed by value,

21–22

Same answer, 4 bytes in size.

26

yes and no, this is an safety... and we may still not see an "throw" if we call function that explicitly declare that throw some exceptions, but we do not have definition.

400
PiotrZSL abandoned this revision.Aug 19 2023, 12:09 PM
PiotrZSL marked 4 inline comments as done.

I do not plan to work on this check anymore, sorry for a wasted time.