DebugInfo lib

[dvyukov, 2012-06-25 14:11:42]

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:68: void addSpecification(const Specification
&spec) { Flags |= spec; }
I would like to use it as:

... = getLineInfoForAddress(addr, DILineInfoSpecifier::flag1 |
DILineInfoSpecifier::flag2 | ...);

I think clear() is excessive. I would make Flags member const.

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:68: void addSpecification(const Specification
&spec) { Flags |= spec; }
No need to pass enum value by const&

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:72: bool needsFunctionName() const {
It does not scale. Perhaps a better option would be:
bool need(Specification s) const;

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:92: const DILineInfoSpecifier &specifier =
DILineInfoSpecifier()) = 0;
No need to pass DILineInfoSpecifier by const&, pass by value.

[samsonov, 2012-06-25 15:09:48]

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:68: void addSpecification(const Specification
&spec) { Flags |= spec; }
On 2012/06/25 14:11:42, dvyukov wrote:
> No need to pass enum value by const&

Done.

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:68: void addSpecification(const Specification
&spec) { Flags |= spec; }
On 2012/06/25 14:11:42, dvyukov wrote:
> I would like to use it as:
> 
> ... = getLineInfoForAddress(addr, DILineInfoSpecifier::flag1 |
> DILineInfoSpecifier::flag2 | ...);
> 
> I think clear() is excessive. I would make Flags member const.

Done.

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:72: bool needsFunctionName() const {
On 2012/06/25 14:11:42, dvyukov wrote:
> It does not scale. Perhaps a better option would be:
> bool need(Specification s) const;

Done.

http://codereview.appspot.com/6342043/diff/4002/include/llvm/DebugInfo/DICont...
include/llvm/DebugInfo/DIContext.h:92: const DILineInfoSpecifier &specifier =
DILineInfoSpecifier()) = 0;
On 2012/06/25 14:11:42, dvyukov wrote:
> No need to pass DILineInfoSpecifier by const&, pass by value.

Done.

[dvyukov, 2012-06-25 15:13:48]

DIContext.h LGTM

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
include/llvm/DebugInfo/DIContext.h:66: DILineInfoSpecifier(uint32_t flags =
FileLineInfo) : Flags(flags) {}
Does
getLineInfoForAddress(0, DILineInfoSpecifier::FileLineInfo |
DILineInfoSpecifier::FunctionName)
compile?

[samsonov, 2012-06-25 15:15:17]

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
include/llvm/DebugInfo/DIContext.h:66: DILineInfoSpecifier(uint32_t flags =
FileLineInfo) : Flags(flags) {}
On 2012/06/25 15:13:48, dvyukov wrote:
> Does
> getLineInfoForAddress(0, DILineInfoSpecifier::FileLineInfo |
> DILineInfoSpecifier::FunctionName)
> compile?

Yes, as I break code style here and don't mark this ctor explicit. See
llvm-dwarfdump.cpp.

[benny.kra_googlemail.com, 2012-06-28 12:19:14]

Sorry, this was on my review list but I didn't get around to it so far.

The approach looks good in general. For the .debug_ranges stuff I'd suggest
looking at LLDB. libDebugInfo's design is strongly inspired by it, and you can
certainly take some clues from it as it is a fast and mature DWARF parser.

One thing that still worries me is the lack of testing (I take most of the blame
here). It seems to be possible now to use binary files as test input (see the
tests in llvm's test/Object), this didn't work reliably a year ago so I didn't
add any tests. It would be nice to have at least a basic test using a binary
object and llvm-dwarfdump, so the code doesn't break as easily.

I hope the process to add tests for libDebugInfo will become easier when
yaml2obj lands, but I don't know the current status of that tool.

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
include/llvm/DebugInfo/DIContext.h:68: return (Flags | spec) > 0;
This is always true if spec is non-zero. Did you mean to use '&'?

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
File lib/DebugInfo/DWARFCompileUnit.cpp (right):

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
lib/DebugInfo/DWARFCompileUnit.cpp:204: if (DieArray.size() > 0) {
The idea here was to skip some work if only the CU was parsed so far.

I'd suggest something like "DieArray.size() > unsigned(keep_compile_unit_die)"
to keep the optimization.

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
lib/DebugInfo/DWARFCompileUnit.cpp:255: }
This function is really specific, it may be better to add a generic way to get a
DIE foor an address out of DWARFCompileUnit. You can take a look at LLDB's
DWARFCompileUnit::LookupAddress if you need some inspiration.

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
File lib/DebugInfo/DWARFDebugAranges.cpp (right):

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
lib/DebugInfo/DWARFDebugAranges.cpp:96: sort(true, /* overlap size */ 0);
Ouch, I guess I never really tested this without an .debug_aranges section
present :(

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugInfo...
File lib/DebugInfo/DWARFDebugInfoEntry.cpp (right):

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugInfo...
lib/DebugInfo/DWARFDebugInfoEntry.cpp:490: }
Those functions look good to me.

[samsonov, 2012-06-29 08:54:39]

On 2012/06/28 12:19:14, bkramer wrote:
> Sorry, this was on my review list but I didn't get around to it so far.
> 
> The approach looks good in general. For the .debug_ranges stuff I'd suggest
> looking at LLDB. libDebugInfo's design is strongly inspired by it, and you can
> certainly take some clues from it as it is a fast and mature DWARF parser.

Thanks for comments and references! I'll take a closer look at interesting parts
of LLDB code.

> One thing that still worries me is the lack of testing (I take most of the
blame
> here). It seems to be possible now to use binary files as test input (see the
> tests in llvm's test/Object), this didn't work reliably a year ago so I didn't
> add any tests. It would be nice to have at least a basic test using a binary
> object and llvm-dwarfdump, so the code doesn't break as easily.

I've added a sketch for the possible test case to the patch (it uses two
pre-built binaries).

> 
> I hope the process to add tests for libDebugInfo will become easier when
> yaml2obj lands, but I don't know the current status of that tool.
> 
>
http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
> File include/llvm/DebugInfo/DIContext.h (right):
> 
>
http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
> include/llvm/DebugInfo/DIContext.h:68: return (Flags | spec) > 0;
> This is always true if spec is non-zero. Did you mean to use '&'?
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
> File lib/DebugInfo/DWARFCompileUnit.cpp (right):
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
> lib/DebugInfo/DWARFCompileUnit.cpp:204: if (DieArray.size() > 0) {
> The idea here was to skip some work if only the CU was parsed so far.
> 
> I'd suggest something like "DieArray.size() > unsigned(keep_compile_unit_die)"
> to keep the optimization.
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
> lib/DebugInfo/DWARFCompileUnit.cpp:255: }
> This function is really specific, it may be better to add a generic way to get
a
> DIE foor an address out of DWARFCompileUnit. You can take a look at LLDB's
> DWARFCompileUnit::LookupAddress if you need some inspiration.
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
> File lib/DebugInfo/DWARFDebugAranges.cpp (right):
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
> lib/DebugInfo/DWARFDebugAranges.cpp:96: sort(true, /* overlap size */ 0);
> Ouch, I guess I never really tested this without an .debug_aranges section
> present :(
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugInfo...
> File lib/DebugInfo/DWARFDebugInfoEntry.cpp (right):
> 
>
http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugInfo...
> lib/DebugInfo/DWARFDebugInfoEntry.cpp:490: }
> Those functions look good to me.

[samsonov, 2012-06-29 08:54:46]

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
File include/llvm/DebugInfo/DIContext.h (right):

http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DICon...
include/llvm/DebugInfo/DIContext.h:68: return (Flags | spec) > 0;
On 2012/06/28 12:19:14, bkramer wrote:
> This is always true if spec is non-zero. Did you mean to use '&'?

Oops. Fixed.

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
File lib/DebugInfo/DWARFCompileUnit.cpp (right):

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
lib/DebugInfo/DWARFCompileUnit.cpp:204: if (DieArray.size() > 0) {
On 2012/06/28 12:19:14, bkramer wrote:
> The idea here was to skip some work if only the CU was parsed so far.
> 
> I'd suggest something like "DieArray.size() > unsigned(keep_compile_unit_die)"
> to keep the optimization.

Done.

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUn...
lib/DebugInfo/DWARFCompileUnit.cpp:255: }
On 2012/06/28 12:19:14, bkramer wrote:
> This function is really specific, it may be better to add a generic way to get
a
> DIE foor an address out of DWARFCompileUnit. You can take a look at LLDB's
> DWARFCompileUnit::LookupAddress if you need some inspiration.

Done. In fact, it may worth to save all the parsed DIEs structure after we
return from this function, as we may do more complex stuff than just fetching
the function name (traverse DIE parents to fetch inlining data, for example).

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
File lib/DebugInfo/DWARFDebugAranges.cpp (right):

http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAran...
lib/DebugInfo/DWARFDebugAranges.cpp:96: sort(true, /* overlap size */ 0);
On 2012/06/28 12:19:14, bkramer wrote:
> Ouch, I guess I never really tested this without an .debug_aranges section
> present :(

BTW it looks like if we want to build a DWARFContext for an executable (not an
object file), we should pass NULL as a pointer to .debug_aranges to be safe - if
we linked together 10 objects, only one of which had the .debug_aranges section,
then the contents of .debug_aranges section in exec won't cover all actual
address ranges.

[dvyukov, 2012-06-29 09:13:57]

On Fri, Jun 29, 2012 at 12:54 PM, <samsonov@google.com> wrote:

>
> http://codereview.appspot.com/**6342043/diff/10004/include/**
>
llvm/DebugInfo/DIContext.h<http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DIContext.h>
> File include/llvm/DebugInfo/**DIContext.h (right):
>
> http://codereview.appspot.com/**6342043/diff/10004/include/**
>
llvm/DebugInfo/DIContext.h#**newcode68<http://codereview.appspot.com/6342043/diff/10004/include/llvm/DebugInfo/DIContext.h#newcode68>
> include/llvm/DebugInfo/**DIContext.h:68: return (Flags | spec) > 0;
> On 2012/06/28 12:19:14, bkramer wrote:
>
>> This is always true if spec is non-zero. Did you mean to use '&'?
>>
>
> Oops. Fixed.
>
>
> http://codereview.appspot.com/**6342043/diff/10004/lib/**
>
DebugInfo/DWARFCompileUnit.cpp<http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUnit.cpp>
> File lib/DebugInfo/**DWARFCompileUnit.cpp (right):
>
> http://codereview.appspot.com/**6342043/diff/10004/lib/**
>
DebugInfo/DWARFCompileUnit.**cpp#newcode204<http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUnit.cpp#newcode204>
> lib/DebugInfo/**DWARFCompileUnit.cpp:204: if (DieArray.size() > 0) {
> On 2012/06/28 12:19:14, bkramer wrote:
>
>> The idea here was to skip some work if only the CU was parsed so far.
>>
>
>  I'd suggest something like "DieArray.size() >
>>
> unsigned(keep_compile_unit_**die)"
>
>> to keep the optimization.
>>
>
> Done.
>
> http://codereview.appspot.com/**6342043/diff/10004/lib/**
>
DebugInfo/DWARFCompileUnit.**cpp#newcode255<http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFCompileUnit.cpp#newcode255>
> lib/DebugInfo/**DWARFCompileUnit.cpp:255: }
>
> On 2012/06/28 12:19:14, bkramer wrote:
>
>> This function is really specific, it may be better to add a generic
>>
> way to get a
>
>> DIE foor an address out of DWARFCompileUnit. You can take a look at
>>
> LLDB's
>
>> DWARFCompileUnit::**LookupAddress if you need some inspiration.
>>
>
> Done. In fact, it may worth to save all the parsed DIEs structure after
> we return from this function, as we may do more complex stuff than just
> fetching the function name (traverse DIE parents to fetch inlining data,
> for example).
>
>
> http://codereview.appspot.com/**6342043/diff/10004/lib/**
>
DebugInfo/DWARFDebugAranges.**cpp<http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAranges.cpp>
> File lib/DebugInfo/**DWARFDebugAranges.cpp (right):
>
> http://codereview.appspot.com/**6342043/diff/10004/lib/**
>
DebugInfo/DWARFDebugAranges.**cpp#newcode96<http://codereview.appspot.com/6342043/diff/10004/lib/DebugInfo/DWARFDebugAranges.cpp#newcode96>
> lib/DebugInfo/**DWARFDebugAranges.cpp:96: sort(true, /* overlap size */
> 0);
> On 2012/06/28 12:19:14, bkramer wrote:
>
>> Ouch, I guess I never really tested this without an .debug_aranges
>>
> section
>
>> present :(
>>
>
> BTW it looks like if we want to build a DWARFContext for an executable
> (not an object file), we should pass NULL as a pointer to .debug_aranges
> to be safe - if we linked together 10 objects, only one of which had the
> .debug_aranges section, then the contents of .debug_aranges section in
> exec won't cover all actual address ranges.



This may slowdown symbolization significantly. I think it's better to
consult .debug_aranges first and then fallback to full parsing.
lldb has the same problem.

[bkramer, 2012-06-30 09:34:43]

LGTM!

[samsonov, 2012-07-02 06:09:48]

On 2012/06/30 09:34:43, bkramer wrote:
> LGTM!

Submitted as r159512, thanks!