lsan for LLVM bootstrap; leaks in TableGen

Hi,

We are trying to enable LeakSanitizer on our asan/msan llvm bootstrap bot
(http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/).
In clang itself there are two leaks
(http://llvm.org/bugs/show_bug.cgi?id=18318, http://llvm-reviews.chandlerc.com/D2472)
and one lsan-hostile feature (http://llvm.org/bugs/show_bug.cgi?id=18320),
all of which are easy to fix.

And there are also lots of leaks in TableGen.
Would anyone be interested in fixing TableGen leaks?
I’d guess not since TableGen is not a user-facing utility, leaks there are not too harmful.
If so, I’d like to disable lsan for TableGen so that
we can enable lsan on the bootstrap bot soon. Objections?

–kcc

Index: utils/TableGen/TableGen.cpp

Its generally been by design that tblgen leaks. Might be nice to fix one day but it’s been that way for a while now so don’t expect it to be fixed soon.

If thats the suppression mechanism of choice (I would’ve expected a build flag option) for lsan, I’d say go for it.

(Maybe there’s some existing build flag handling for valgrind so as not to leak check tblgen, but that would be a runtime flag, not build time)

Its generally been by design that tblgen leaks. Might be nice to fix one day
but it's been that way for a while now so don't expect it to be fixed soon.

If thats the suppression mechanism of choice (I would've expected a build
flag option) for lsan, I'd say go for it.

(Maybe there's some existing build flag handling for valgrind so as not to
leak check tblgen, but that would be a runtime flag, not build time)

Hi,

We are trying to enable LeakSanitizer on our asan/msan llvm bootstrap bot

(http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/).

In clang itself there are two leaks

(http://llvm.org/bugs/show_bug.cgi?id=18318,
http://llvm-reviews.chandlerc.com/D2472)

and one lsan-hostile feature (http://llvm.org/bugs/show_bug.cgi?id=18320),

all of which are easy to fix.

And there are also lots of leaks in TableGen.

I think the problem with TableGen is that we call leak checking too
late - after the global destructors,
not before them. IIRC pointers to objects "leaked" in TableGen are
stored in a set with static storage duration.
If we're able to call __lsan_do_leak_check() right after main(), we
will treat that objects as reachable.

I considered adding a call to __lsan_do_leak_check into
llvm_shutdown() function, but that looks wrong -
for instance, llvm_shutdown() may be called when a dynamic library is unloaded.

Its generally been by design that tblgen leaks. Might be nice to fix one
day but it's been that way for a while now so don't expect it to be fixed
soon.

If thats the suppression mechanism of choice

It is.

(I would've expected a build flag option)

That would not be trivial.
The link-time flag is -fsanitize=address, and we do want to pass it because
we do want to check TableGen for addressability bugs
(and because if we don't pass it at link time we can not use it at compile
time, which will cause a huge cmake/make challenge).
Inventing a link time flag that will keep asan but remove lsan is not worth
it.
There is the run-time flag (ASAN_OPTIONS=detect_leaks=0|1) which turns lsan
on/off,
but again passing it selectively to TableGen is harder than my simple
patch.

for lsan, I'd say go for it.

Ok, unless someone objects.

--kcc

> Its generally been by design that tblgen leaks. Might be nice to fix one
day
> but it's been that way for a while now so don't expect it to be fixed
soon.
>
> If thats the suppression mechanism of choice (I would've expected a build
> flag option) for lsan, I'd say go for it.
>
> (Maybe there's some existing build flag handling for valgrind so as not
to
> leak check tblgen, but that would be a runtime flag, not build time)
>
>
> Hi,
>
> We are trying to enable LeakSanitizer on our asan/msan llvm bootstrap bot
>
> (http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/).
>
> In clang itself there are two leaks
>
> (http://llvm.org/bugs/show_bug.cgi?id=18318,
> http://llvm-reviews.chandlerc.com/D2472)
>
> and one lsan-hostile feature (http://llvm.org/bugs/show_bug.cgi?id=18320
),
>
> all of which are easy to fix.
>
> And there are also lots of leaks in TableGen.

I think the problem with TableGen is that we call leak checking too
late - after the global destructors,
not before them. IIRC pointers to objects "leaked" in TableGen are
stored in a set with static storage duration.
If we're able to call __lsan_do_leak_check() right after main(), we
will treat that objects as reachable.

This is part of the problem. Indeed, there is code like this:

void foo() {
  static Pool ThePool;
   ...
  ThePool.Insert(new Stuff);
  ...
}

This static variable is destructed w/o deleting the contents and lsan
complains.
I've replaced some of these objects with
  static Pool *ThePool = new Pool
and those leaks were gone.

But then there were a few other leaks of different nature, all of which I
did not want to investigate.

--kcc

LGTM.

I think it is totally reasonable to just disable LSan directly in tablegen at least for now.

I would leave some comments on the disabling to clarify:

  1. What it does, the reader may not have heard about LSan.
  2. Some of the high-level information from this thread as pointers for anyone who wants to go and fix this some day.

-Chandler

Like this?

+extern “C” {
+// Disable LeakSanitizer, see http://llvm.org/bugs/show_bug.cgi?id=18325.
+int __lsan_is_turned_off() { return 1; }
+} // extern “C”

We don't often reference bugs in comments. I would give a brief summary in
the text of the comment, and mention the bug in the commit log.

This?

+extern “C” {
+// Disable LeakSanitizer for this binary as it has too many leaks that are not
+// very interesting to fix. __lsan_is_turned_off is explained in
+// compiler-rt/include/sanitizer/lsan_interface.h
+int __lsan_is_turned_off() { return 1; }
+} // extern “C”

Thoughts?

LGTM

[ Sorry to be joining the conversation late ]

What is the reasoning behind having them define a function to disable lsan, rather than calling __lsan_disable?
Is it so that lsan can be turned off before main() is entered?

I’m not really happy with the idea of the user having to define a function with a reserved name in their code.

— Marshall

Like this?

+extern "C" {
+// Disable LeakSanitizer, see
http://llvm.org/bugs/show_bug.cgi?id=18325.

We don't often reference bugs in comments. I would give a brief summary
in the text of the comment, and mention the bug in the commit log.

This?

+extern "C" {
+// Disable LeakSanitizer for this binary as it has too many leaks that
are not
+// very interesting to fix. __lsan_is_turned_off is explained in
+// compiler-rt/include/sanitizer/lsan_interface.h
+int __lsan_is_turned_off() { return 1; }
+} // extern “C"

[ Sorry to be joining the conversation late ]

What is the reasoning behind having them define a function to disable
lsan, rather than calling __lsan_disable?
Is it so that lsan can be turned off before main() is entered?

Yes. Also, this allows us to disable lsan w/o modifying the source file
where main() is defined

--kcc

FTR: LLVM is now LeakSanitizer-clean, lsan is enabled on the sanitizer bootstrap bot, which is green:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap

–kcc