[llvm] r246406 - [dsymutil] Fix handling of inlined_subprogram low_pcs
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 8 22:09:42 PDT 2015
On Tue, Sep 8, 2015 at 1:10 PM, Frédéric Riss <friss at apple.com> wrote:
>
> On Sep 8, 2015, at 12:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Aug 31, 2015 at 11:10 AM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On Aug 31, 2015, at 9:07 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Mon, Aug 31, 2015 at 9:05 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Sun, Aug 30, 2015 at 6:43 PM, Frederic Riss via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: friss
>>>> Date: Sun Aug 30 20:43:14 2015
>>>> New Revision: 246406
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=246406&view=rev
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D246406-26view-3Drev&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=-yp_b9w-sonxhFICg6npPkz6_FLOw29qR_X8EIzjwWY&e=>
>>>> Log:
>>>> [dsymutil] Fix handling of inlined_subprogram low_pcs
>>>>
>>>> The value of an inlined subprogram low_pc attribute should not
>>>> get relocated, but it can happen that it matches the enclosing
>>>> function's start address and thus gets the generic treatment.
>>>> Special case it to avoid applying the PC offset twice.
>>>>
>>>
>>> I'm a tad confused - do you store the low_pcs as offsets relative to the
>>> function
>>>
>>
>> (sorry, bouncy shuttle to work & accidentally sent before I finished that
>> sentence...)
>>
>> do you store the low_pcs as offsets relative to the function's low_pc?
>> That's interesting - and perhaps something we should standardize/generalize
>> to reduce relocations in all our DWARF output (but I don't think there's
>> any standard for it yet in DWARF), but I'm not sure why that would require
>> special casing the case where the two low_pcs are equal - wouldn't that
>> just mean the low_pc of the inlined subroutine would be at zero offset from
>> the subprogram's low_pc? (& still not relocated)
>>
>>
>> dsymutil takes the debug map as input that only contains the function
>> (and variables) start addresses. That’s the only thing we can count on
>> being exact. We then do a pass over all the debug_info relocations to find
>> the ones that correspond to those addresses (and the DIEs where we find the
>> ‘interesting’ relocations are the ones that define which part of the DIE
>> tree we keep). Then — once we decided what to keep — we go over the kept
>> DIEs and we clone them, applying the relocations in the process. But note
>> that the relocations we’ve chosen are only for the entry points, thus we
>> need to have the code around to handle the
>> lexical_block/inlined_subroutine, and this code doesn’t use the relocations
>> (it applies an offset that we computed when handling the subprogram DIE).
>>
>> What happened here is that the generic code that applied the relocations
>> would also patch the inlined_subroutine low_pc because the relocation was
>> the same as the entry point. And then the code handling the low_pc
>> attributes for the inlined_subroutine would apply the offset a second time.
>>
>
> OK - what I'm wondering is whether it would work better/as well to
> generalize this code, rather than two distinct passes/processes.
>
>
> I don’t think there’s a way to generalize this code. But I agree that
> storing the low_/high_pcs as offsets from their enclosing function low_pc
> would save quite a few relocations.
>
Sorry, that wasn't what I was trying to describe, but it's certainly
something we've discussed before (actually I made a silly prototype of
using dwarf expressions and debug address pool indicies to do reloc sharing
(using one reloc per section (macho would use one reloc per function, due
to the implied function-sections like behavior) - never did get around to
running good numbers on it, though)).
> Note that there is precedent for something like this: the ranges are
> encoded as offsets from the *CU* low_pc. Maybe it would be more natural to
> use that then?
>
If we had a (probably/preferably compact) encoding to describe this, it
would probably be ideal.
DWARF4 already has this /sort/ of thing for high_pc (where it can be
encoded as a static offset relative to the low_pc - so it's not another
relocation). That could possibly be generalized further to allow high_pcs
to be a static offset relative to their enclosing high_pc (if one exists,
otherwise it would be an unacceptable encoding (this could occur for
functions - if the CU isn't a contiguous PC range (non-CU functions in
between CU functions, functions in other sections, etc) or if a function
itself is discontiguous (hot/cold code splitting)).
Eric & I have bandied that around now & then, which lead to the
aforementioned prototype I played around with, but didn't go any further
than that - my improvements to Clang's debug info emission had already
brought it down to half the size of GCC's, so we didn't have any particular
need to push further at the time.
>
> low_pc should just be a zero-offset relocation, right?
>
>
> Not on mach-o. Most relocations will be of the form __text+offset. That’s
> why there is no way for me to differentiate a __text+offset references the
> end of a range from the exact same relocation that references the beginning
> of another one (and as the linker can tear apart sections, that distinction
> is fundamental).
>
OK, so you search through looking for a subprogram that has a subprogram
low_pc at __text+offset? then assume all the other low/high pcs (and
ranges) are relative to that function starting point? (this is how you
remove the ambiguity of the start/end?)
>
> Maybe I'm not understanding/explaining very well, though.
>
>
>> We might be able to completely remove any specific handling and just
>> ‘promote’ all the relocations that fall inside a linked function as
>> interesting. At the point we do that triaging relocs, we are not exploring
>> the DIE tree though, just looking at the relocation list, so it would
>> require us to trust the size field of the debug map, and I’m not sure we
>> can do that 100% of the time (I know that this field is not accurate, it’s
>> usually too big because it factors in alignment, but that might not be an
>> issue if nothing gets allocated in the alignment padding).
>>
>
> Hmm - not sure I follow this. You're suggesting that if a non-debug-aware
> tool applied the relocations in the object file/debug info, it would
> mangle/damage the debug info?
>
>
> Basically yes. As I explain above a relocation based off the __text
> section with a constant offset could be replaced by different values
> depending on the context. I already said that, but I guess the message is
> hard to get through: dsymutil uses the object file relocations to know what
> to link, but it doesn’t do relocation processing in the usual sense,
> because this simply wouldn’t work (More precisely, it tries to do as much
> standard relocation processing as possible, but it needs some code to
> workaround the cases where that logic gives the wrong result).
>
It's slowly sinking in, I appreciate your patience in (repeatedly)
explaining it to me.
- Dave
>
> Fred
>
> - David
>
>
>>
>> Fred
>>
>>
>>>
>>>>
>>>> Added:
>>>> llvm/trunk/test/tools/dsymutil/ARM/dummy-debug-map-amr64.map
>>>> llvm/trunk/test/tools/dsymutil/ARM/inlined-low_pc.c
>>>> llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/
>>>> llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/1.o
>>>> Modified:
>>>> llvm/trunk/test/tools/dsymutil/ARM/lit.local.cfg
>>>> llvm/trunk/tools/dsymutil/DwarfLinker.cpp
>>>>
>>>> Added: llvm/trunk/test/tools/dsymutil/ARM/dummy-debug-map-amr64.map
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/ARM/dummy-debug-map-amr64.map?rev=246406&view=auto
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_tools_dsymutil_ARM_dummy-2Ddebug-2Dmap-2Damr64.map-3Frev-3D246406-26view-3Dauto&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=Za5qJ6LsFqNTEnS7lR9nOUQu3xiakQs49dpO89XaJg0&e=>
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/tools/dsymutil/ARM/dummy-debug-map-amr64.map (added)
>>>> +++ llvm/trunk/test/tools/dsymutil/ARM/dummy-debug-map-amr64.map Sun
>>>> Aug 30 20:43:14 2015
>>>> @@ -0,0 +1,15 @@
>>>> +# This is a dummy debug map used for some tests where the contents of
>>>> the
>>>> +# map are just an implementation detail. The tests wanting to use that
>>>> file
>>>> +# should put all there object files in an explicitely named
>>>> sub-directory
>>>> +# of Inputs, and they should be named 1.o, 2.o, ...
>>>> +# As not finding an object file or symbols isn't a fatal error for
>>>> dsymutil,
>>>> +# you can extend this file with as much object files and symbols as
>>>> needed.
>>>> +
>>>> +---
>>>> +triple: 'arm64-apple-darwin'
>>>> +objects:
>>>> + - filename: 1.o
>>>> + symbols:
>>>> + - { sym: _bar, objAddr: 0x0, binAddr: 0x10000, size: 0x10 }
>>>> +...
>>>> +
>>>>
>>>> Added: llvm/trunk/test/tools/dsymutil/ARM/inlined-low_pc.c
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/ARM/inlined-low_pc.c?rev=246406&view=auto
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_tools_dsymutil_ARM_inlined-2Dlow-5Fpc.c-3Frev-3D246406-26view-3Dauto&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=Q8OeSlNX91218xjgr3aIrN1U1Bssvnu-Nu8WzoX_nlI&e=>
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/tools/dsymutil/ARM/inlined-low_pc.c (added)
>>>> +++ llvm/trunk/test/tools/dsymutil/ARM/inlined-low_pc.c Sun Aug 30
>>>> 20:43:14 2015
>>>> @@ -0,0 +1,15 @@
>>>> +/* Compiled with: clang -arch=arm64 -O2 -g -c inlined_low_pc.c */
>>>> +
>>>> +static int foo(int i) { return 42 + i; }
>>>> +int bar(int a) { return foo(a); }
>>>> +
>>>> +// RUN: llvm-dsymutil -f -y %p/dummy-debug-map-amr64.map
>>>> -oso-prepend-path %p/../Inputs/inlined-low_pc -o - | llvm-dwarfdump - |
>>>> FileCheck %s
>>>> +
>>>> +// CHECK: DW_TAG_subprogram
>>>> +// CHECK: DW_AT_low_pc{{.*}}0x0000000000010000
>>>> +// CHECK: DW_AT_name{{.*}}"bar"
>>>> +// CHECK-NOT: NULL
>>>> +// CHECK: DW_TAG_inlined_subroutine
>>>> +// CHECK-NEXT: DW_AT_abstract_origin{{.*}}"foo"
>>>> +// CHECK-NEXT: DW_AT_low_pc{{.*}}0x0000000000010000
>>>> +
>>>>
>>>> Modified: llvm/trunk/test/tools/dsymutil/ARM/lit.local.cfg
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/ARM/lit.local.cfg?rev=246406&r1=246405&r2=246406&view=diff
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_tools_dsymutil_ARM_lit.local.cfg-3Frev-3D246406-26r1-3D246405-26r2-3D246406-26view-3Ddiff&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=zEAxxhhWFPrzEd5HWOml9qsDlLRCwN45LHyqj9f5wUc&e=>
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/tools/dsymutil/ARM/lit.local.cfg (original)
>>>> +++ llvm/trunk/test/tools/dsymutil/ARM/lit.local.cfg Sun Aug 30
>>>> 20:43:14 2015
>>>> @@ -2,3 +2,6 @@ if not 'ARM' in config.root.targets:
>>>> config.unsupported = True
>>>> if not 'AArch64' in config.root.targets:
>>>> config.unsupported = True
>>>> +
>>>> +config.suffixes = ['.test', '.cpp', '.c']
>>>> +
>>>>
>>>> Added: llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/1.o
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/1.o?rev=246406&view=auto
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_tools_dsymutil_Inputs_inlined-2Dlow-5Fpc_1.o-3Frev-3D246406-26view-3Dauto&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=lY2aOkTZ45aHfHwE3xL8Amnff5hUDIrveaBrWrrNdnI&e=>
>>>>
>>>> ==============================================================================
>>>> Binary files llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/1.o
>>>> (added) and llvm/trunk/test/tools/dsymutil/Inputs/inlined-low_pc/1.o Sun
>>>> Aug 30 20:43:14 2015 differ
>>>>
>>>> Modified: llvm/trunk/tools/dsymutil/DwarfLinker.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/DwarfLinker.cpp?rev=246406&r1=246405&r2=246406&view=diff
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_tools_dsymutil_DwarfLinker.cpp-3Frev-3D246406-26r1-3D246405-26r2-3D246406-26view-3Ddiff&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=6xjIRngxynhJS54wLMKfMmHyjNR49UDteCNLX-SlUxI&e=>
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/dsymutil/DwarfLinker.cpp (original)
>>>> +++ llvm/trunk/tools/dsymutil/DwarfLinker.cpp Sun Aug 30 20:43:14 2015
>>>> @@ -1206,6 +1206,7 @@ private:
>>>> const char *Name, *MangledName; ///< Names.
>>>> uint32_t NameOffset, MangledNameOffset; ///< Offsets in the string
>>>> pool.
>>>>
>>>> + uint64_t OrigLowPc; ///< Value of AT_low_pc in the input DIE
>>>> uint64_t OrigHighPc; ///< Value of AT_high_pc in the input DIE
>>>> int64_t PCOffset; ///< Offset to apply to PC addresses inside a
>>>> function.
>>>>
>>>> @@ -1214,8 +1215,8 @@ private:
>>>>
>>>> AttributesInfo()
>>>> : Name(nullptr), MangledName(nullptr), NameOffset(0),
>>>> - MangledNameOffset(0), OrigHighPc(0), PCOffset(0),
>>>> HasLowPc(false),
>>>> - IsDeclaration(false) {}
>>>> + MangledNameOffset(0), OrigLowPc(UINT64_MAX), OrigHighPc(0),
>>>> + PCOffset(0), HasLowPc(false), IsDeclaration(false) {}
>>>> };
>>>>
>>>> /// \brief Helper for cloneDIE.
>>>> @@ -2274,7 +2275,12 @@ unsigned DwarfLinker::cloneAddressAttrib
>>>> if (AttrSpec.Attr == dwarf::DW_AT_low_pc) {
>>>> if (Die.getTag() == dwarf::DW_TAG_inlined_subroutine ||
>>>> Die.getTag() == dwarf::DW_TAG_lexical_block)
>>>> - Addr += Info.PCOffset;
>>>> + // The low_pc of a block or inline subroutine might get
>>>> + // relocated because it happens to match the low_pc of the
>>>> + // enclosing subprogram. To prevent issues with that, always use
>>>> + // the low_pc from the input DIE if relocations have been
>>>> applied.
>>>> + Addr = (Info.OrigLowPc != UINT64_MAX ? Info.OrigLowPc : Addr) +
>>>> + Info.PCOffset;
>>>> else if (Die.getTag() == dwarf::DW_TAG_compile_unit) {
>>>> Addr = Unit.getLowPc();
>>>> if (Addr == UINT64_MAX)
>>>> @@ -2522,6 +2528,11 @@ DIE *DwarfLinker::cloneDIE(const DWARFDe
>>>> // high_pc value is done in cloneAddressAttribute().
>>>> AttrInfo.OrigHighPc =
>>>> InputDIE.getAttributeValueAsAddress(&U, dwarf::DW_AT_high_pc,
>>>> 0);
>>>> + // Also store the low_pc. It might get relocated in an
>>>> + // inline_subprogram that happens at the beginning of its
>>>> + // inlining function.
>>>> + AttrInfo.OrigLowPc =
>>>> + InputDIE.getAttributeValueAsAddress(&U, dwarf::DW_AT_low_pc,
>>>> UINT64_MAX);
>>>> }
>>>>
>>>> // Reset the Offset to 0 as we will be working on the local copy of
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=_sX2G1Du1KZyzi5BD4_ddw&m=FkrVlXa3-EdUHEUklJrpTIxLR2zDdr3ysgnj0hyNiNc&s=pIHV3NLou6W4R8NtF6A-1IkCPvKIdG-XzjtGugnY6Ts&e=>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150908/ea1c8154/attachment.html>
More information about the llvm-commits
mailing list