Szelethus added a comment.
In https://reviews.llvm.org/D45532#1145512, @sylvestre.ledru wrote:
> If you are interested, I executed this checker on Firefox code. Results can
> be found here
> http://sylvestre.ledru.info/reports/fx-scan-build/
Absolutely, thank you! :)
> Andi reported this
george.karpenkov added a comment.
@sylvestre.ledru Have you found any actual bugs using this checker?
@Szelethus Interesting, it seems that the pointer itself is initialized, but
not what it's pointing to.
I think we should just check the fields directly, and do not attempt to
traverse the
sylvestre.ledru added a comment.
If you are interested, I executed this checker on Firefox code. Results can be
found here
http://sylvestre.ledru.info/reports/fx-scan-build/
Andi reported this bug https://bugs.llvm.org/show_bug.cgi?id=37965
Repository:
rC Clang
This revision was automatically updated to reflect the committed changes.
Closed by commit rC334935: [analyzer] Checker for uninitialized C++ objects
(authored by Szelethus, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D45532
Files:
Szelethus updated this revision to Diff 151685.
Szelethus added a comment.
Whoo! Thank you all for helping me with the reviews. Very excited about the
upcoming fixes, I have some neat ideas for some already.
- Rebased to 8186139d6aa0619e2057ae617c074e486a7b2f2b (revision 334929),
- Added a
NoQ accepted this revision.
NoQ added a comment.
Ok, let's land this one and see how it goes! I'm looking forward to seeing the
follow-up patches.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+ // Checking bases.
+ const auto *CXXRD =
Szelethus updated this revision to Diff 150899.
Szelethus added a comment.
Polite ping :)
This diff fixed a minor issue in FieldChainInfo's constructor.
https://reviews.llvm.org/D45532
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
Szelethus marked 5 inline comments as done.
Szelethus added a comment.
In https://reviews.llvm.org/D45532#1116992, @george.karpenkov wrote:
> @Szelethus I personally really like this idea, and I think it would be a
> great checker.
> Could you perform more evaluation on other projects?
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+ ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+ if (!Node)
+return;
Szelethus wrote:
> NoQ wrote:
> > Szelethus
Szelethus updated this revision to Diff 149290.
Szelethus added a comment.
- Rebased to 1cefbc5593d2f017ae56a853b0723a31865aa602 (revision 333276)
- Fixed some typos
- Added tests `CyclicPointerTest` and `CyclicVoidPointerTest` to highlight an
issue to be fixed in a later patch
george.karpenkov added a comment.
@Szelethus I personally really like this idea, and I think it would be a great
checker.
Could you perform more evaluation on other projects?
https://reviews.llvm.org/D45532
___
cfe-commits mailing list
Szelethus marked 4 inline comments as done.
Szelethus added a comment.
I decided to mark the inline comments in `isPointerOrReferenceUninit` regarding
the dereferencing loop termination done for now. I left several TODO's in the
function to be fixed later, with many of them depending on one
Szelethus marked 9 inline comments as done.
Szelethus added a comment.
Thanks again for taking a look :)
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+ // Checking bases.
+ const auto *CXXRD = dyn_cast(RD);
+ if (!CXXRD)
+return
NoQ added a comment.
Ok! Putting any remaining work into follow-up patches would be great indeed.
Let's land this into `alpha.cplusplus` for now because i still haven't made up
my mind for how to organize the proposed `bugprone` category (sorry!).
Comment at:
Szelethus marked 28 inline comments as done.
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:181-182
+/// Returns whether FD can be (transitively) dereferenced to a void pointer
type
+/// (void*, void**, ...). The type of
Szelethus updated this revision to Diff 147485.
Szelethus added a comment.
In this diff, I preferred placing `TODO`s in places where the logic of the
checker would have changed too much. While I didn't have a strict rule for this
in mind, I tried changing as little as possible in order to
Szelethus added a comment.
I'm afraid it'll be even more time before I post a new diff. There are some
things that `ProgramState` is lacking, so I'll get that fixed first in a
different pull request first.
Comment at:
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+Report->addNote(NoteBuf,
+PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+
Szelethus added a comment.
Thanks @NoQ for taking the time to review my code!
I'm sorry that the patch became this long. It was an error on my part, and I
completely get that the checker now takes an uncomfortably long time to read
and understand. This is my first "bigger" project in the CSA,
NoQ added a comment.
Had a look at the actual code, came up with some comments, most are fairly
minor, so good job! You did a good job explaining the overall idea, but a lot
of specifics were difficult to understand, so i made a few comments on how to
make the code a bit easier to read.
> I
whisperity added a comment.
Two minor comments.
With regards to the function naming, the problem with incremental counts is
that they get out of sync, like they did with `fXpY` and such, and if someone
wants to keep the count incremental down the file, adding any new test will
result in an
Szelethus updated this revision to Diff 10.
Szelethus added a comment.
Forgot to rename the system header simulator file. Sorry about the spam :)
https://reviews.llvm.org/D45532
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
Szelethus marked an inline comment as done.
Szelethus added inline comments.
Comment at: test/Analysis/cxx-uninitialized-object.cpp:526
+
+void f23p5() {
+ void *vptr = malloc(sizeof(int));
I haven't marked @a.sidorin's comment as done, but it disappeared
Szelethus updated this revision to Diff 144439.
Szelethus marked an inline comment as done.
Szelethus added a comment.
Renamed the checker to `cplusplus.uninitialized.UninitializedObject`.
I've read your comments regarding the category this should be in thoroughly,
and should the clang-tidy
Szelethus marked 3 inline comments as done.
Szelethus added a comment.
By the way, thank you all for taking the time to review my code!
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:306
+
+ const RecordDecl *RD =
+
Szelethus updated this revision to Diff 144119.
Szelethus added a comment.
Fixes according to inline comments.
https://reviews.llvm.org/D45532
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
a.sidorin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:72
+ const FieldDecl *getEndOfChain() const { return Chain.back()->getDecl(); }
+ template void toString(SmallString ) const;
+ friend struct FieldChainInfoComparator;
Szelethus updated this revision to Diff 143875.
Szelethus added a comment.
In this diff I
- added a `Pedantic` flag that is set to false by default to filter out results
from objects that don't have a single field initialized,
- made it so that fields that are declared in system headers are now
Szelethus added a comment.
I'd also like to point out that as I mentioned before, the checker's name
itself is misleading (it is a leftover from an earlier implementation of this
checker). Here are just some ideas I came up with:
- UninitializedObjectChecker
- UninitializedFieldsChecker
-
NoQ added a comment.
Guys, what do you think about a checker that warns on uninitialized fields only
when at least one field is initialized? I'd be much more confident about
turning such check on by default. We can still keep a `pedantic` version.
> I can say with confidence that CodeChecker
whisperity added a comment.
@george.karpenkov @NoQ `bugprone.` as a category sounds nice. It also nicely
corresponds to the Clang-Tidy `bugprone-` category. It would not be nice to
further fragment the "top levels" of checker categories.
I can say with confidence that CodeChecker does not
george.karpenkov added a comment.
`antipatterns` or `security` could be another potential category name.
https://reviews.llvm.org/D45532
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.karpenkov added a comment.
The code looks good apart from a few minor nits. I think I would prefer a new
category created for this checker instead of using `alpha`; let's see what @NoQ
has to say.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:305
+
Szelethus updated this revision to Diff 142760.
Szelethus added a comment.
In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:
> Also, I managed to cause a crash with the class `linked_ptr_internal` from
> google's boringssl when I analyzed the grpc project. I'll look deeper into
>
MTC added a comment.
In https://reviews.llvm.org/D45532#1069683, @whisperity wrote:
> There is something that came up in my mind:
>
> Consider a construct like this:
>
> class A
> {
> A()
> {
> memset(X, 0, 10 * sizeof(int));
> }
>
> int X[10];
> };
>
>
> I think
whisperity requested changes to this revision.
whisperity added a comment.
This revision now requires changes to proceed.
There is something that came up in my mind:
Consider a construct like this:
class A
{
A()
{
memset(X, 0, 10 * sizeof(int));
}
int X[10];
};
I
whisperity added a comment.
In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:
> In https://reviews.llvm.org/D45532#1068647, @dkrupp wrote:
>
> > This bug report also mentions assignment operator. But for that a warning
> > may be not so useful. In that case the members of the
Szelethus updated this revision to Diff 142624.
Szelethus added a comment.
> Would be interesting to extend this checker (maybe in an upcoming patch) to
> report on uninitialized members not only in constructors, but also copy
> constructors and move constructors.
Added 3 new test cases. to
dkrupp added a comment.
Would be interesting to extend this checker (maybe in an upcoming patch) to
report on uninitialized members not only in constructors, but also copy
constructors and move constructors.
See related https://bugs.llvm.org/show_bug.cgi?id=37086
This bug report also mentions
Szelethus marked 24 inline comments as done.
Szelethus added a comment.
Note that there was a comment made about the test files being too long. I still
haven't split them, as I didn't find a good "splitting point". Is this okay, or
shall I try to split these into numerous smaller ones?
Szelethus updated this revision to Diff 142581.
Szelethus marked 2 inline comments as done.
Szelethus edited the summary of this revision.
Szelethus added a comment.
Among many other things:
- The checker class is now on top of the file.
- Reviewed all comments, fixed typos, tried to make the
Szelethus marked 6 inline comments as done.
Szelethus added a comment.
I'm about to update the diff, I changed a quite a lot of stuff, so I'm not sure
that I'd be able to respond to these inline comments.
Comment at:
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:35
+
+ FieldChain Chain;
+ // If this is a fieldchain whose last element is an uninitialized region of a
I was wondering, do you need the chain at all?
MTC added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:10
+//
+// This file defines a checker that checks cunstructors for possibly
+// uninitialized fields.
typo :) `cunstructors` -> `constructors`
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+ if (isCalledByConstructor(Context))
+return;
xazax.hun wrote:
> Szelethus wrote:
> > whisperity wrote:
> > > I think somewhere there should
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+class FieldChainInfo {
As far as I
Szelethus added inline comments.
Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+ : NonPolymorphicLeft1(int{}) {
+y = 420;
+z = 420;
Szelethus wrote:
> whisperity wrote:
> > The literal `420` is repeated //everywhere// in
Szelethus added a comment.
Thank you for all your comments so far! I'll probably only be able to update
the diff tomorrow (with me being in the GMT + 1 timezone).
> That's a lot of code, so it'll take me some time to understand what's going
> on here. You might be able to help me by the large
whisperity requested changes to this revision.
whisperity added subscribers: gsd, dkrupp, o.gyorgy.
whisperity added a comment.
This revision now requires changes to proceed.
Sorry, one comment has gone missing meanwhile, I'm still getting used to this
interface and hit //Submit// early.
whisperity added a comment.
@NoQ Do you reckon these tests files are too long? Perhaps the one about this
inheritance, that inheritance, diamond inheritance, etc. could be split into
multiple files.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
+def
NoQ added a comment.
In https://reviews.llvm.org/D45532#1064685, @Szelethus wrote:
> In https://reviews.llvm.org/D45532#1064670, @Szelethus wrote:
>
> > I wasn't too clear on this one: unknown fields are regarded as
> > uninitialized, what I wrote was a temporary change in the code so I could
Szelethus added a comment.
In https://reviews.llvm.org/D45532#1064670, @Szelethus wrote:
> I wasn't too clear on this one: unknown fields are regarded as uninitialized,
> what I wrote was a temporary change in the code so I could check whether it
> would work.
Woops, I meant that unknown
NoQ added a comment.
Ok, thanks, looking forward to it!
Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+ ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized
field}}
+}
Szelethus wrote:
> NoQ wrote:
> > Hmm,
Szelethus added a comment.
In https://reviews.llvm.org/D45532#1064652, @NoQ wrote:
> > In most cases, all fields of a union is regarded as unknown. I checked
> > these cases by regarding unknown fields as uninitialized.
>
> The whole point of making them unknown, as far as i understand, was to
Szelethus added inline comments.
Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+ ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized
field}}
+}
NoQ wrote:
> Hmm, shouldn't it say "expected"? Do these tests
NoQ added a comment.
That's a lot of code, so it'll take me some time to understand what's going on
here. You might be able to help me by the large patch in smaller logical chunks.
The idea looks reasonable. We're having specific warnings for, eg., assigning
an uninitialized value, which fire
Szelethus updated this revision to Diff 142053.
Szelethus added a comment.
Reuploaded the diff with full context.
https://reviews.llvm.org/D45532
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lebedev.ri added a comment.
Please always upload all patches with full context (`-U9`)
Repository:
rC Clang
https://reviews.llvm.org/D45532
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, dkrupp, whisperity.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet, mgorny.
Herald added a reviewer: george.karpenkov.
This checker checks at the end of a constructor call whether all fields of the
60 matches
Mail list logo
