[llvm] r252825 - [ARM] Enable shrink-wrapping by default.

Tue Nov 17 09:24:06 PST 2015

Thanks Evgenii!

> On Nov 16, 2015, at 5:05 PM, Evgenii Stepanov <eugenis at google.com> wrote:
> 
> On Mon, Nov 16, 2015 at 4:10 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> 
>>> On Nov 16, 2015, at 3:16 PM, Evgenii Stepanov <eugenis at google.com <mailto:eugenis at google.com>> wrote:
>>> 
>>> I've reverted 253116 locally, but can not reproduce the problem.
>> 
>> You also need to run with (-mllvm) -enable-shrink-wrap
> 
> Thanks, this works.
> 
>> 
>>> 
>>> Anyway, it sounds like debug information is broken in part of the
>>> function, i.e. the memory access is attributed to the calling function
>>> - this should be visible in a debugger.
>> 
>> I don’t get what you mean here. Could you give an example?
> 
> This is what I think is going on:
> On x86, a "naked" function is described like this:
>  DW_CFA_def_cfa: r7 (rsp) ofs 8
>  DW_CFA_offset: r16 (rip) at cfa-8
> 
> On ARM though, return address is in a register, and until its pushed
> on the stack, there is no way (or is there?) to describe it in DWARF:
>  DW_CFA_def_cfa: r13 ofs 0
> 
> ^ this uses r13 from the caller frame, so unwind instructions from
> that frame will be used. That's why we see the stack trace without
> main(), where some libc function immediately follows NullDeref().
> 
> The same can be reproduced without shrink wrapping on this code:
> 
> int f() {   return 1;  }
> 
> It will use unwind instructions from the underlying frame.
> 
> Sounds like this is just a platform limitation and does not break ASan
> in any case that really matter, so I'd suggest turning it back and
> tweaking the test.

Interesting, that would mean will would need to somehow emit naked-like debug information prior to the actual setting of the frame pointer.
I am wondering though, is it possible to have several description of the CFA within the same function?
I admit I am out of my domain of expertise here.

> 
>> 
>>> 
>>> Unrelated: all compilations in the failing test use
>>> -fno-omit-frame-pointer; with shrink wrapping part of the function is
>>> run without a valid frame pointer. Is it OK?
>> 
>> I guess that's arguable, but basically right now with shrink-wrapping you get the frame pointer only when it is required, i.e., in that case before the call to the runtime (before the fix).
>> That means that yes the frame is not valid elsewhere, but this is also why we get the performance improvements: we don’t set/unset the frame on leaf functions.
>> 
>> The motivating examples are:
>> 
>> foo () {
>> if (…)
>>   errorout()
>> ...
>> return
>> }
> 
> This could be fine. ASan requires that -fno-omit-frame-pointer enables
> fp-based stack unwind from any ASan runtime library call. It sounds
> like shrink wrapping would preserve this property (i.e. any external
> function call site would have a proper frame with a proper frame
> pointer).

Yes, it does. My concern is that even on x86, the test case that failed for ARM also failed for x86 due to the lack of description of the CFA prior to the setting of the frame pointer.
I am going to look into this debug info setting to see what I can see.

Thanks again for the inputs!

Cheers,
-Quentin

> 
>> 
>>> 
>>> 
>>> On Fri, Nov 13, 2015 at 6:05 PM, Evgenii Stepanov <eugenis at google.com> wrote:
>>>> I'm not sure without taking a closer look. Maybe next week.
>>>> 
>>>> On Fri, Nov 13, 2015 at 6:03 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>> Hi Evgenii,
>>>>> 
>>>>> Thanks for the inputs.
>>>>> 
>>>>> I’ve disabled shrink-wrapping for functions with the sanitize like
>>>>> attributes in Committed revision 253116.
>>>>> My understanding is that the frame information must be correct at the
>>>>> location of the crash, which can happen anywhere, not just at what
>>>>> shrink-wrapping considers as needing the frame to be lowered.
>>>>> 
>>>>> Does that make sense or did I just create a workaround for something I
>>>>> didn’t understand?
>>>>> 
>>>>> Thanks,
>>>>> -Quentin
>>>>> 
>>>>> On Nov 12, 2015, at 12:59 PM, Evgenii Stepanov <eugenis at google.com> wrote:
>>>>> 
>>>>> Looks like debug location for prologue code could be broken or
>>>>> missing. See EntryDebugLocation in
>>>>> FunctionStackPoisoner::poisonStack() in AddressSanitizer.cpp.
>>>>> 
>>>>> On Thu, Nov 12, 2015 at 12:32 PM, Quentin Colombet <qcolombet at apple.com>
>>>>> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I’ve had a quick look and the generated code looks good to me.
>>>>> The prologue code in the failing example is needed only just before the call
>>>>> to the asan function (see attached assembly for x86).
>>>>> 
>>>>> I am guessing that ASAN has some heuristics to find the prologue code in the
>>>>> disassembly and that fails for the new code.
>>>>> Note that the debugger just get it right!
>>>>> 
>>>>> Could you advise on what to do?
>>>>> 
>>>>> Thanks,
>>>>> Q
>>>>> 
>>>>> 
>>>>> 
>>>>> On Nov 12, 2015, at 9:45 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>> 
>>>>> Hi Renato,
>>>>> 
>>>>> Thanks for the follow-up.
>>>>> 
>>>>> I’ll try to have a look as well, unless you beat me at it :).
>>>>> 
>>>>> Cheers,
>>>>> -Quentin
>>>>> 
>>>>> On Nov 12, 2015, at 5:35 AM, Renato Golin <renato.golin at linaro.org> wrote:
>>>>> 
>>>>> On 11 November 2015 at 23:31, Quentin Colombet via llvm-commits
>>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>> 
>>>>> Author: qcolombet
>>>>> Date: Wed Nov 11 17:31:46 2015
>>>>> New Revision: 252825
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=252825&view=rev
>>>>> Log:
>>>>> [ARM] Enable shrink-wrapping by default.
>>>>> 
>>>>> Differential Revision: http://reviews.llvm.org/D14357
>>>>> 
>>>>> rdar://problem/21942589
>>>>> 
>>>>> 
>>>>> Quentin,
>>>>> 
>>>>> After bisecting, this seems to have caused:
>>>>> 
>>>>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9014
>>>>> 
>>>>> http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/9014/steps/ninja%20check%201/logs/FAIL%3A%20AddressSanitizer-armhf-linux%3A%3Anull_deref.cc
>>>>> 
>>>>> I'm copying sanitizer folks to understand what's the change here, but
>>>>> I'm guessing the sanitizer instrumentation didn't get inserted right,
>>>>> with the prologue changed.
>>>>> 
>>>>> I'll revert for now...
>>>>> 
>>>>> cheers,
>>>>> --renato

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151117/406e6229/attachment-0001.html>