[PATCH] Tests and interface for LeakSanitizer.

Sergey Matveev earthdok at google.com
Mon Apr 22 04:33:02 PDT 2013



================
Comment at: lib/lsan/lsan_interface.h:40
@@ +39,3 @@
+void SetSources(uptr sources);
+void SetPointerAlignment(uptr alignment);
+
----------------
Kostya Serebryany wrote:
> Sergey Matveev wrote:
> > Kostya Serebryany wrote:
> > > Sergey Matveev wrote:
> > > > Kostya Serebryany wrote:
> > > > > Why do you need these functions at all? 
> > > > > Why not just two more parameters to ReportLeaks? 
> > > > > 
> > > > It makes sense, but the readability of the tests would suffer. The use of masks has already complicated simple operations like "flip one source flag and keep the rest unchanged". Now we would also have to specify pointer alignment everywhere -- even though there's only one test that has anything to do with it. Besides, internally I'm storing those flags in global state anyway (in non-test usage they're not supposed to change after initialization, so there's no sense in passing them into every function that uses them). So the functions wouldn't go away -- I'd just move the call to Set{Sources,PointerAlignment} into ReportLeaks.
> > > you don't need alignment as a separate argument either. 
> > > There are only two choices that makes sence: the pointers are either word-aligned or not. 
> > > So, this should be like another flag kSourceUnaligned. If it's not set -- you don't use it. 
> > > The global state would simply store these flags as is.
> > Ok, that makes a lot of sense. At this point I think lsan_testing_interface.h should be merged into lsan.h, but that will have to wait until I commit the latter.
> > 
> > Also, we could save on some code duplication if ReportLeaks used InternalVector instead of a fixed size array, so IsLeaked and IsNotLeaked could be merged into one function. But I'm uncertain whether it's worth complicating the interface.
> >> lsan_testing_interface.h should be merged into lsan.h
> Agreed. 
> We could have two variants of ReportLeaks, one with vector, one with array. 
Ok, I removed the array version entirely for now since no one uses it.

Can I get an LGTM now?


http://llvm-reviews.chandlerc.com/D677

BRANCH
  lsan_interface_and_tests

ARCANIST PROJECT
  compiler-rt



More information about the llvm-commits mailing list