Patchwork [asan] migrate runtime from llvm

login
register
mail settings
Submitter Wei Mi
Date Oct. 20, 2012, 1:41 a.m.
Message ID <CA+4CFy7jQJTcZ4eD=a-U3e6Kug5QuhMT9B5_6YrQAOx7K6sLHg@mail.gmail.com>
Download mbox | patch
Permalink /patch/192885/
State New
Headers show

Comments

Wei Mi - Oct. 20, 2012, 1:41 a.m.
Thanks David. Here is the patch after removing m4 directory under libasan.
And I add the dependence libasan on libstdc++-v3 to avoid problem
under parallel making for now.

The patch is too big even after compressed, so I have to split the
patch into two parts contained in two mails. patch.part1.txt and
patch.part2.txt. This is the first part.

 // generated by the libgomp configure.  Unfortunately, due to the use of
 //  recursive make, we can't be that specific.

Thanks,
Wei.

On Fri, Oct 19, 2012 at 12:52 PM, Xinliang David Li <davidxl@google.com> wrote:
> The library builds fine with the following diff. The file acinclude.m4
> is cloned from libmudflap.
>
> David
>
> index 485d169..3e847f1 100644
> --- a/libasan/aclocal.m4
> +++ b/libasan/aclocal.m4
> @@ -1037,8 +1037,8 @@ AC_SUBST([am__tar])
>  AC_SUBST([am__untar])
>  ]) # _AM_PROG_TAR
>
> -m4_include([m4/libtool.m4])
> -m4_include([m4/ltoptions.m4])
> -m4_include([m4/ltsugar.m4])
> -m4_include([m4/ltversion.m4])
> -m4_include([m4/lt~obsolete.m4])
> +m4_include([../config/ltoptions.m4])
> +m4_include([../config/ltsugar.m4])
> +m4_include([../config/ltversion.m4])
> +#m4_include([../config/lt~obsolete.m4])
> +m4_include([acinclude.m4])
>
>
> On Fri, Oct 19, 2012 at 11:02 AM, Wei Mi <wmi@google.com> wrote:
>> David, I put the m4 subdir under libasan because once I use the .m4
>> files (libtool.m4  lt~obsolete.m4  ltoptions.m4  ltsugar.m4
>> ltversion.m4) and ltmain.sh under $topsrcdir, the problem that a bad
>> libtool was generated under
>> $topbuilddir/x86_64-unknown-linux-gnu/libasan you met yesterday
>> appeared.  That is why I had to generate the new libtool m4 files and
>> ltmain.sh using libtoolize.
>>
>> Thanks,
>> Wei.
>>
>> On Fri, Oct 19, 2012 at 10:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> I tried it, and this version works for me.
>>>
>>> Your probably do not need to add the m4 subdir under libasan.  The
>>> required m4 files are either in .. or ../config dir. See how
>>> libmudflap does it.
>>>
>>> Other than that, if there are no other comments, the change is good to
>>> check into the branch. Remaining bugs can always be found and fixed
>>> later.
>>>
>>> thanks,
>>>
>>> David
>>>
>>>
>>>
>>> On Thu, Oct 18, 2012 at 8:04 PM, Wei Mi <wmi@google.com> wrote:
>>>> Hi,
>>>>
>>>> David cought a problem in the last patch when he tries to built
>>>> libasan. The problem was that an incomplete libtool under libasan
>>>> build directory was generated. The cause is that the old patch used an
>>>> old ltmain.sh to generate libtool. I fix it and attach a new patch.
>>>> And the new patch move -lpthread and -ldl to libasan LDFLAGS.
>>>>
>>>> Thanks,
>>>> Wei.
Wei Mi - Oct. 20, 2012, 1:42 a.m.
And this is the second part.

Thanks,
Wei.

On Fri, Oct 19, 2012 at 6:41 PM, Wei Mi <wmi@google.com> wrote:
> Thanks David. Here is the patch after removing m4 directory under libasan.
> And I add the dependence libasan on libstdc++-v3 to avoid problem
> under parallel making for now.
>
> The patch is too big even after compressed, so I have to split the
> patch into two parts contained in two mails. patch.part1.txt and
> patch.part2.txt. This is the first part.
>
> --- a/Makefile.def      2012-10-19 15:36:36.156106282 -0700
> +++ b/Makefile.def      2012-10-19 15:36:51.656186869 -0700
> @@ -504,7 +504,6 @@
>  dependencies = { module=configure-target-libobjc;
> on=configure-target-boehm-gc; };
>  dependencies = { module=all-target-libobjc; on=all-target-boehm-gc; };
>  dependencies = { module=configure-target-libstdc++-v3;
> on=configure-target-libgomp; };
> +dependencies = { module=configure-target-libasan;
> on=all-target-libstdc++-v3; };
>  // parallel_list.o and parallel_settings.o depend on omp.h, which is
>  // generated by the libgomp configure.  Unfortunately, due to the use of
>  //  recursive make, we can't be that specific.
>
> Thanks,
> Wei.
>
> On Fri, Oct 19, 2012 at 12:52 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The library builds fine with the following diff. The file acinclude.m4
>> is cloned from libmudflap.
>>
>> David
>>
>> index 485d169..3e847f1 100644
>> --- a/libasan/aclocal.m4
>> +++ b/libasan/aclocal.m4
>> @@ -1037,8 +1037,8 @@ AC_SUBST([am__tar])
>>  AC_SUBST([am__untar])
>>  ]) # _AM_PROG_TAR
>>
>> -m4_include([m4/libtool.m4])
>> -m4_include([m4/ltoptions.m4])
>> -m4_include([m4/ltsugar.m4])
>> -m4_include([m4/ltversion.m4])
>> -m4_include([m4/lt~obsolete.m4])
>> +m4_include([../config/ltoptions.m4])
>> +m4_include([../config/ltsugar.m4])
>> +m4_include([../config/ltversion.m4])
>> +#m4_include([../config/lt~obsolete.m4])
>> +m4_include([acinclude.m4])
>>
>>
>> On Fri, Oct 19, 2012 at 11:02 AM, Wei Mi <wmi@google.com> wrote:
>>> David, I put the m4 subdir under libasan because once I use the .m4
>>> files (libtool.m4  lt~obsolete.m4  ltoptions.m4  ltsugar.m4
>>> ltversion.m4) and ltmain.sh under $topsrcdir, the problem that a bad
>>> libtool was generated under
>>> $topbuilddir/x86_64-unknown-linux-gnu/libasan you met yesterday
>>> appeared.  That is why I had to generate the new libtool m4 files and
>>> ltmain.sh using libtoolize.
>>>
>>> Thanks,
>>> Wei.
>>>
>>> On Fri, Oct 19, 2012 at 10:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> I tried it, and this version works for me.
>>>>
>>>> Your probably do not need to add the m4 subdir under libasan.  The
>>>> required m4 files are either in .. or ../config dir. See how
>>>> libmudflap does it.
>>>>
>>>> Other than that, if there are no other comments, the change is good to
>>>> check into the branch. Remaining bugs can always be found and fixed
>>>> later.
>>>>
>>>> thanks,
>>>>
>>>> David
>>>>
>>>>
>>>>
>>>> On Thu, Oct 18, 2012 at 8:04 PM, Wei Mi <wmi@google.com> wrote:
>>>>> Hi,
>>>>>
>>>>> David cought a problem in the last patch when he tries to built
>>>>> libasan. The problem was that an incomplete libtool under libasan
>>>>> build directory was generated. The cause is that the old patch used an
>>>>> old ltmain.sh to generate libtool. I fix it and attach a new patch.
>>>>> And the new patch move -lpthread and -ldl to libasan LDFLAGS.
>>>>>
>>>>> Thanks,
>>>>> Wei.
Xinliang David Li - Oct. 25, 2012, 4:24 p.m.
To accommodate tsan (and msan in the future), the directory structure
needs to be changed. The top level directory can be called something
like librt with the following subdirs: asan, tsan, sanitizer_common,
and interception.  Also the libasan should be linked in statically by
default.

thanks,

David

On Fri, Oct 19, 2012 at 6:42 PM, Wei Mi <wmi@google.com> wrote:
> And this is the second part.
>
> Thanks,
> Wei.
>
> On Fri, Oct 19, 2012 at 6:41 PM, Wei Mi <wmi@google.com> wrote:
>> Thanks David. Here is the patch after removing m4 directory under libasan.
>> And I add the dependence libasan on libstdc++-v3 to avoid problem
>> under parallel making for now.
>>
>> The patch is too big even after compressed, so I have to split the
>> patch into two parts contained in two mails. patch.part1.txt and
>> patch.part2.txt. This is the first part.
>>
>> --- a/Makefile.def      2012-10-19 15:36:36.156106282 -0700
>> +++ b/Makefile.def      2012-10-19 15:36:51.656186869 -0700
>> @@ -504,7 +504,6 @@
>>  dependencies = { module=configure-target-libobjc;
>> on=configure-target-boehm-gc; };
>>  dependencies = { module=all-target-libobjc; on=all-target-boehm-gc; };
>>  dependencies = { module=configure-target-libstdc++-v3;
>> on=configure-target-libgomp; };
>> +dependencies = { module=configure-target-libasan;
>> on=all-target-libstdc++-v3; };
>>  // parallel_list.o and parallel_settings.o depend on omp.h, which is
>>  // generated by the libgomp configure.  Unfortunately, due to the use of
>>  //  recursive make, we can't be that specific.
>>
>> Thanks,
>> Wei.
>>
>> On Fri, Oct 19, 2012 at 12:52 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The library builds fine with the following diff. The file acinclude.m4
>>> is cloned from libmudflap.
>>>
>>> David
>>>
>>> index 485d169..3e847f1 100644
>>> --- a/libasan/aclocal.m4
>>> +++ b/libasan/aclocal.m4
>>> @@ -1037,8 +1037,8 @@ AC_SUBST([am__tar])
>>>  AC_SUBST([am__untar])
>>>  ]) # _AM_PROG_TAR
>>>
>>> -m4_include([m4/libtool.m4])
>>> -m4_include([m4/ltoptions.m4])
>>> -m4_include([m4/ltsugar.m4])
>>> -m4_include([m4/ltversion.m4])
>>> -m4_include([m4/lt~obsolete.m4])
>>> +m4_include([../config/ltoptions.m4])
>>> +m4_include([../config/ltsugar.m4])
>>> +m4_include([../config/ltversion.m4])
>>> +#m4_include([../config/lt~obsolete.m4])
>>> +m4_include([acinclude.m4])
>>>
>>>
>>> On Fri, Oct 19, 2012 at 11:02 AM, Wei Mi <wmi@google.com> wrote:
>>>> David, I put the m4 subdir under libasan because once I use the .m4
>>>> files (libtool.m4  lt~obsolete.m4  ltoptions.m4  ltsugar.m4
>>>> ltversion.m4) and ltmain.sh under $topsrcdir, the problem that a bad
>>>> libtool was generated under
>>>> $topbuilddir/x86_64-unknown-linux-gnu/libasan you met yesterday
>>>> appeared.  That is why I had to generate the new libtool m4 files and
>>>> ltmain.sh using libtoolize.
>>>>
>>>> Thanks,
>>>> Wei.
>>>>
>>>> On Fri, Oct 19, 2012 at 10:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> I tried it, and this version works for me.
>>>>>
>>>>> Your probably do not need to add the m4 subdir under libasan.  The
>>>>> required m4 files are either in .. or ../config dir. See how
>>>>> libmudflap does it.
>>>>>
>>>>> Other than that, if there are no other comments, the change is good to
>>>>> check into the branch. Remaining bugs can always be found and fixed
>>>>> later.
>>>>>
>>>>> thanks,
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>
>>>>> On Thu, Oct 18, 2012 at 8:04 PM, Wei Mi <wmi@google.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> David cought a problem in the last patch when he tries to built
>>>>>> libasan. The problem was that an incomplete libtool under libasan
>>>>>> build directory was generated. The cause is that the old patch used an
>>>>>> old ltmain.sh to generate libtool. I fix it and attach a new patch.
>>>>>> And the new patch move -lpthread and -ldl to libasan LDFLAGS.
>>>>>>
>>>>>> Thanks,
>>>>>> Wei.
Jakub Jelinek - Oct. 25, 2012, 4:39 p.m.
On Thu, Oct 25, 2012 at 09:24:51AM -0700, Xinliang David Li wrote:
> To accommodate tsan (and msan in the future), the directory structure
> needs to be changed. The top level directory can be called something
> like librt with the following subdirs: asan, tsan, sanitizer_common,
> and interception.  Also the libasan should be linked in statically by
> default.

librt is a very bad name, that clashes with glibc librt, would only create
confusion.

Why should be libasan linked statically by default?

	Jakub
Xinliang David Li - Oct. 25, 2012, 4:48 p.m.
On Thu, Oct 25, 2012 at 9:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 25, 2012 at 09:24:51AM -0700, Xinliang David Li wrote:
>> To accommodate tsan (and msan in the future), the directory structure
>> needs to be changed. The top level directory can be called something
>> like librt with the following subdirs: asan, tsan, sanitizer_common,
>> and interception.  Also the libasan should be linked in statically by
>> default.
>
> librt is a very bad name, that clashes with glibc librt, would only create
> confusion.

Ok, then we should pick something that is not confusing but reflect
the fact  they are for runtime error checking ..

>
> Why should be libasan linked statically by default?
>

There are a couple of reasons:

1) it makes running sanitized binary on remote machines which does not
have libasan installed easier;
2) There is no guarantee that libasan API won't change, statically
linking it in makes it less vulnerable to such changes.

thanks,

David

>         Jakub
Diego Novillo - Oct. 25, 2012, 4:52 p.m.
On Thu, Oct 25, 2012 at 12:48 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Oct 25, 2012 at 9:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> librt is a very bad name, that clashes with glibc librt, would only create
>> confusion.
>
> Ok, then we should pick something that is not confusing but reflect
> the fact  they are for runtime error checking ..

libsanitizer?


Diego.
Xinliang David Li - Oct. 25, 2012, 4:54 p.m.
On Thu, Oct 25, 2012 at 9:52 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Oct 25, 2012 at 12:48 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Thu, Oct 25, 2012 at 9:39 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> librt is a very bad name, that clashes with glibc librt, would only create
>>> confusion.
>>
>> Ok, then we should pick something that is not confusing but reflect
>> the fact  they are for runtime error checking ..
>
> libsanitizer?

+1 --- missed the obvious one :)

David

>
>
> Diego.
Jakub Jelinek - Oct. 25, 2012, 4:55 p.m.
On Thu, Oct 25, 2012 at 09:48:54AM -0700, Xinliang David Li wrote:
> > Why should be libasan linked statically by default?

> There are a couple of reasons:
> 
> 1) it makes running sanitized binary on remote machines which does not
> have libasan installed easier;
> 2) There is no guarantee that libasan API won't change, statically
> linking it in makes it less vulnerable to such changes.

Neither of the reasons look important enough, compared to the
http://www.akkadia.org/drepper/no_static_linking.html
Especially as libasan is overriding malloc etc., having dozens of malloc
overrides in different shared libraries, perhaps all chaining into each
other, is a nightmare.
For 2), libasan should just use symbol versioning and provide backwards
compatibility as long as it is possible, or worst case bump SONAME
occassionally.

	Jakub
Xinliang David Li - Oct. 25, 2012, 5 p.m.
On Thu, Oct 25, 2012 at 9:55 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 25, 2012 at 09:48:54AM -0700, Xinliang David Li wrote:
>> > Why should be libasan linked statically by default?
>
>> There are a couple of reasons:
>>
>> 1) it makes running sanitized binary on remote machines which does not
>> have libasan installed easier;
>> 2) There is no guarantee that libasan API won't change, statically
>> linking it in makes it less vulnerable to such changes.
>
> Neither of the reasons look important enough, compared to the
> http://www.akkadia.org/drepper/no_static_linking.html
> Especially as libasan is overriding malloc etc., having dozens of malloc
> overrides in different shared libraries, perhaps all chaining into each
> other, is a nightmare.


How about statically linking just for executables, not shared library buid?

David

> For 2), libasan should just use symbol versioning and provide backwards
> compatibility as long as it is possible, or worst case bump SONAME
> occassionally.
>
>         Jakub
Jakub Jelinek - Oct. 25, 2012, 5:19 p.m.
On Thu, Oct 25, 2012 at 10:00:03AM -0700, Xinliang David Li wrote:
> How about statically linking just for executables, not shared library buid?

That is IMHO still a bad idea.

	Jakub
Xinliang David Li - Oct. 25, 2012, 5:24 p.m.
On Thu, Oct 25, 2012 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 25, 2012 at 10:00:03AM -0700, Xinliang David Li wrote:
>> How about statically linking just for executables, not shared library buid?
>
> That is IMHO still a bad idea.

I don't know why you think so (It seems that the points mentioned in
http://www.akkadia.org/drepper/no_static_linking.html mainly apply to
release binaries, not sanitized ones), but for now let's drop the
static link request.

thanks,

David


>
>         Jakub
Diego Novillo - Oct. 25, 2012, 5:27 p.m.
On Thu, Oct 25, 2012 at 1:24 PM, Xinliang David Li <davidxl@google.com> wrote:

> I don't know why you think so (It seems that the points mentioned in
> http://www.akkadia.org/drepper/no_static_linking.html mainly apply to
> release binaries, not sanitized ones), but for now let's drop the
> static link request.

Yeah, we can always just do that in our own builds.


Diego.

Patch

--- a/Makefile.def      2012-10-19 15:36:36.156106282 -0700
+++ b/Makefile.def      2012-10-19 15:36:51.656186869 -0700
@@ -504,7 +504,6 @@ 
 dependencies = { module=configure-target-libobjc;
on=configure-target-boehm-gc; };
 dependencies = { module=all-target-libobjc; on=all-target-boehm-gc; };
 dependencies = { module=configure-target-libstdc++-v3;
on=configure-target-libgomp; };
+dependencies = { module=configure-target-libasan;
on=all-target-libstdc++-v3; };
 // parallel_list.o and parallel_settings.o depend on omp.h, which is