[PATCH] D31905: Exit early from start line search for FunctionNameKind::None

[David Blaikie via llvm-commits]

On Tue, Apr 11, 2017 at 6:18 AM Brian Cain <bcain at codeaurora.org> wrote:

> The resulting breakage is only to the feature that’s new as of r294231, or
> does it impact more than that?
>
>
>
> Could we do this partial revert first and then in a subsequent commit add
> the code to support both use cases?
>

It's been in for a couple of months now (yeah, it's a pretty vague line
where to change from "revert early/revert often" to "fix forward" - but I'm
leaning the latter at this point) so I'd prefer to fix forward if possible.

Looks like it'd be a weird regression for llvm-symbolizer to have the
function start line dropped because the function name info wasn't
requested. (they seem to me like orthogonal features)

So probably best to update the test case added in r294231 to include a
variant that switches off function name resolution, but shows the function
start line is still printed. Then preserve that functionality/test while
fixing your issue - probably with an extra flag down through
DILineInfoSpecifier.


>
>
> -Brian
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Monday, April 10, 2017 3:53 PM
> *To:* reviews+D31905+public+410e06a7b468ba68 at reviews.llvm.org;
> bcain at codeaurora.org; sque at google.com; echristo at gmail.com
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* Re: [PATCH] D31905: Exit early from start line search for
> FunctionNameKind::None
>
>
>
> I would guess this would then cause the line to not be retrieved when it
> might be desired? But I haven't thought about it too hard (hopefully Simon
> can chime in).
>
> If that's the case & the existing functionality needs to be preserved,
> then maybe two functions will be needed or some other way to communicate
> "actually I don't need the line" (pass it by pointer instead of reference -
> null pointer indicates "not needed")
>
>
>
> On Mon, Apr 10, 2017 at 1:46 PM Brian Cain via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> bcain created this revision.
>
> https://reviews.llvm.org/rL294231 introduced a regression -- memory
> consumption for llvm-objdump during disassembly of ~500MB hexagon elf file
> was ~1.5GiB higher after that commit.
>
> This change restores the previous behavior -- exit the search early for
> FunctionNameKind::None, saving some memory allocation.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D31905
>
> Files:
>   lib/DebugInfo/DWARF/DWARFContext.cpp
>
>
> Index: lib/DebugInfo/DWARF/DWARFContext.cpp
> ===================================================================
> --- lib/DebugInfo/DWARF/DWARFContext.cpp
> +++ lib/DebugInfo/DWARF/DWARFContext.cpp
> @@ -466,6 +466,9 @@
>                                                    FunctionNameKind Kind,
>                                                    std::string
> &FunctionName,
>                                                    uint32_t &StartLine) {
> +  if (Kind == FunctionNameKind::None)
> +    return false;
> +
>    // The address may correspond to instruction in some inlined function,
>    // so we have to build the chain of inlined functions and take the
>    // name of the topmost function in it.
> @@ -477,7 +480,7 @@
>    const DWARFDie &DIE = InlinedChain[0];
>    bool FoundResult = false;
>    const char *Name = nullptr;
> -  if (Kind != FunctionNameKind::None && (Name =
> DIE.getSubroutineName(Kind))) {
> +  if ((Name = DIE.getSubroutineName(Kind))) {
>      FunctionName = Name;
>      FoundResult = true;
>    }
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170411/efa5b203/attachment.html>