Re: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Maxim Ostapenko <m dot ostapenko at samsung dot com>]

+ Jakub

On 09/08/17 19:21, Jeff Law wrote:
> On 08/08/2017 12:46 AM, Vyacheslav Barinov wrote:
> > Hello,
> >
> > Andrew Pinski <pinskia@gmail.com> writes:
> >
> > > On Mon, Aug 7, 2017 at 11:15 PM, Slava Barinov <v.barinov@samsung.com> wrote:
> > > >         gcc/
> > > >         * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
> > > >
> > > >         gcc/testsuite/
> > > >         * g++.dg/asan/global-alignment.cc: New test to test global
> > > >         variables alignment.
> > >
> > > Can you describe this a little bit more?  What is going wrong here?
> > > Is it because there is no red zone between the variables?
> > I've described situation in bugzilla PR 81697 with compiled files dumps, but
> > briefly yes, red zone size is incorrect between global variables.
> >
> > On certain platforms (we checked at least arm and ppc) the flow is following:
> > make_decl_rtl() calls create_block_symbol() on decl tree with ASan global
> > variable describing a string. But then get_variable_section() returns wrong
> > section (.rodata.str1.4 in my case) because check in asan_protect_global()
> > returns false due to !DECL_RTL_SET_P check.
> >
> > When variable is placed not into .rodata, but into .rodata.str ld treats it as
> > string and just shrinks the large part of red zone silently (gold at least
> > prints warning about wrong string alignment). But in run time libasan expects
> > that red zone is still there and reports false positives.
> >
> > In order to prevent the setting of RTL before ASan handling I tried to
> > reproduce the x86_64 behavior and found that use_object_blocks_p() returns
> > false for that platform. Accordingly to the function comment it reports if
> > 'current compilation mode benefits from grouping', and just switching it off
> > for any ASan builds resolves the issue.
> >
> > > Also I noticed you are using .cc as the testcase file name, why don't
> > > you use .C instead and then you won't need the other patch which you
> > > just posted.
> > Okay, attaching rebased version.
> In many ways it'd be better if asan could be made to work with section
> anchors.  Doing so means the code we're running for asan more closely
> matches what we're running in the normal case.

I'm not very familiar with this code, but as far as I can understand, 
the problem is that we have a circular dependency here -- 
categorize_decl_for_section calls asan_protect_global that returns false 
because DECL_RTL is not set at this point and assigns decl to 
SECCAT_RODATA_MERGE_CONST section. But when make_decl_rtl sets up 
SET_DECL_RTL (decl, x) and following asan_protect_global calls will 
return true and we end up with mismatch between section and alignment 
requirements. Perhaps we can add an additional flag to 
asan_protect_global to change its behavior in such cases (e.g. don't 
account for DECL_RTL_SET_P)? In this case we will change the section for 
SECCAT_RODATA without affecting global section anchors flag.

-Maxim

> But I'll defer to Jakub and Marek as they know the sanitizers far better
> than I do and are more familiar with the expected tradeoffs.
>
> jeff