On Tue, Oct 16, 2012 at 3:03 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Tue, Oct 16, 2012 at 02:41:42PM -0700, Xinliang David Li wrote:
>> > +bool
>> > +asan_protect_global (tree decl)
>> > +{
>> > + rtx rtl, symbol;
>> > + section *sect;
>> > +
>> > + if (TREE_CODE (decl) != VAR_DECL
>> > + || DECL_THREAD_LOCAL_P (decl)
>> > + || DECL_EXTERNAL (decl)
>> > + || !TREE_ASM_WRITTEN (decl)
>> > + || !DECL_RTL_SET_P (decl)
>> > + || DECL_ONE_ONLY (decl)
>> > + || DECL_COMMON (decl)
>>
>> Why the above two condition? If the linker picks the larger size one,
>> it is ok to do the instrumentation.
>
> For DECL_ONE_ONLY, what LLVM does is plain wrong, it puts address of
> even vars exported from shared libraries (which can be overridden) into
> the array passed to __asan_*register_globals. That can't work, you don't
> know if the var that is found first was compiled with -fasan or not.
> We need to use a local alias in that case (yeah, my WIP patch doesn't do
> that yet, but I wanted to post at least something), and I believe local
Does that mean that all globals defined in shared libraries can not be
protected as long as they are not protected or hidden? This sounds
like a big limitation. We need to answer the following two questions:
1) How often are exported variables get preempted?
2) Is it a common use case to mix -fasan and -fno-asan ?
If the answer is no for either of the questions, we should allow the
above at the risk of some possible false positives -- as the goal is
to find as many bugs as possible (without too much noise). In short,
false negatives are worse.
> aliases might be an issue with DECL_ONE_ONLY (and anyway, if a -fno-asan
> DECL_ONE_ONLY var wins over -fasan one, there is no padding after it).
>
> LLVM does other things wrong, it increases the size of the vars which is
> IMHO a big no no, say for copy relocs etc., and last I'd say the relocations
> in the description variable are unnecessary, would be better if it e.g. used
> PC relative relocations and could made the array passed to
> __asan_*register_globals read-only.
>
I like the GCC way better too.
> And for DECL_COMMON, you can't put any padding after a common variable
> without making the size of the common var larger (and increasing its
> alignment), both are undesirable for -fasan/-fno-asan mixing.
If the linker picks the large one (which I believe it does), is that
still a problem?
>
>> > + || (DECL_SECTION_NAME (decl) != NULL_TREE
>> > + && !DECL_HAS_IMPLICIT_SECTION_NAME_P (decl))
>>
>> Why is this condition? Is it related to -fdata-sections ?
>
> -fdata-sections will have non-NULL DECL_SECTION_NAME, but still
> DECL_HAS_IMPLICIT_SECTION_NAME_P. The above condition is not to break
> various packages that put say a struct into some user section and expect
> the section then to contain an array of those structs.
> E.g. Linux kernel does this, systemtap probes, prelink, ...
> If padding is inserted, all those would break.
>
Ok -- not common scenarios to be of a concern.
thanks,
David
> Jakub
