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

Mon Nov 16 17:05:19 PST 2015

On Mon, Nov 16, 2015 at 4:10 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>> On Nov 16, 2015, at 3:16 PM, Evgenii Stepanov <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.

>
>>
>> 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).

>
>>
>>
>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>