Allow specifications to refer to other specifications that follow them in

Allow specifications to refer to other specifications that follow them in
the debug info.

BUG=441
TEST=dwarf_cu_to_module_unittest (including new tests)
     Run dump_syms on a binary produced by clang and it should work.

[jimb]

Yeah, that's the kind of second pass you'd need to do. Obviously, you need real
tests; you need to observe the style guide; perhaps there are places where you
should have a copy constructor; and so on. But the idea looks right.

If you're going to do this second pass, I'm really wondering if there isn't a
lot of other stuff that could be simplified. That file is really hard to follow.
("Who wrote this crap? ... Oh.")

I really won't be able to review this properly until September --- I'm very
sorry.

[Mark Mentovai]

OK, in the absence of a real review, let me just ask you this one additional
question:

dump_syms also complains about DW_AT_abstract_origin forward references. As best
as I can tell, it seems that dump_syms tracks abstract origins but never
actually uses them. Is this intentional, or can I get rid of the tracking and
associated warnings? (Or did I misread, and are they actually used?)

[Mark Mentovai]

I’ve uploaded a version that handles the abstract origin stuff, passes the
tests, and adds a test for the forward-reference stuff. It’s still dirty and
needs cleanup, but I wanted to snapshot it. Ted, Jim said he wouldn’t be able to
review rapidly. Do you think you’d have a chance to review it once it’s clean?

[Mark Mentovai]

I’ve uploaded a clean version and this is now ready for review.

[Mark Mentovai]

I’m going to have to check in a local copy of these files to Chrome to unblock
our compiler work. This is obviously not desirable, and I’d like to move back to
the in-Breakpad-tree versions as soon as possible. But for the time being, it’s
a tiny bit less pressure on you.

[thakis]

Ping? :-)

[Ted Mielczarek]

Rafael thinks he fixed this in LLVM with this commit:
http://llvm.org/viewvc/llvm-project?view=rev&revision=144441

Can you test and see if this fixes your issues?

[jimb]

On 2011/11/14 15:58:26, Ted Mielczarek wrote:
> Rafael thinks he fixed this in LLVM with this commit:
> http://llvm.org/viewvc/llvm-project?view=rev&revision=144441
> 
> Can you test and see if this fixes your issues?

I don't see anything in Rafael's patch that seems like it would cause DIEs to
appear in a different order. That's the issue here.

[jimb]

Thanks very much for working this through! It's great to have something like
Specification playing the role of representing what we need from the
specification DIEs we encounter.

http://breakpad.appspot.com/299001/diff/8010/10001
File common/dwarf_cu_to_module.cc (left):

http://breakpad.appspot.com/299001/diff/8010/10001#oldcode92
Line 92: typedef map<uint64, AbstractOrigin> AbstractOriginByOffset;
Great to see these two things unified into one type.

http://breakpad.appspot.com/299001/diff/8010/10001
File common/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/299001/diff/8010/10001#newcode61
Line 61: // offset key in the DwarfCUToModule::FilePrivate::specifications map.
Subclassing Function doesn't seem like a good idea. The base class doesn't have
a virtual destructor, and Modules "own" the Functions added to them via
AddFunction, so if a subclass of Function ever acquired a destructor, Module
wouldn't call it correctly.

I think this should just be its own entirely separate type, and we should
construct Module::Function instances in DwarfCUToModule::Finish, since we're
looping over them there already. More on this below.

http://breakpad.appspot.com/299001/diff/8010/10001#newcode98
Line 98: string QualifiedName(DwarfCUToModule::CUContext* cu_context, uint64
offset);
Does this fit with the C++ Style Guide's admonition to use 'struct' only for
things without much behavior?

The comment should say, "cu_context must be the CUContext for the compilation
unit in which this Specification appears", instead of talking about the details
of how it is presently used.

http://breakpad.appspot.com/299001/diff/8010/10001#newcode180
Line 180: // type derived from google_breakpad::Module::Function*.
Module also has an AddFunction method, so we don't need to do weird subtyping
stuff just to keep this type. I'd just change this to
vector<DwarfCUToModule::Function> (no pointer), and then construct
Module::Function instances in DwarfCUToModule::Finish, since we're looping over
them there anyway.

http://breakpad.appspot.com/299001/diff/8010/10001#newcode184
Line 184: string DwarfCUToModule::Specification::QualifiedName(
It would be really nice to have some one-line comments over each major step here
that relate each step to the aspects of DWARF structure that we're dealing with.

http://breakpad.appspot.com/299001/diff/8010/10001#newcode393
Line 393: file_context->file_private->specifications[offset_] = specification;
I think if we just write:

...->specifications[offset_] = Specification(... constructor args ...);

the compiler will Do The Right Thing.

http://breakpad.appspot.com/299001/diff/8010/10001#newcode972
Line 972: 0, function->specification_offset);
Could we avoid repeating this code and improve clarity by abstracting this out
into a "find Specification or warn and return NULL pointer" function?

http://breakpad.appspot.com/299001/diff/8010/10002
File common/dwarf_cu_to_module.h (right):

http://breakpad.appspot.com/299001/diff/8010/10002#newcode229
Line 229: typedef map<uint64, Specification*> SpecificationByOffset;
From what I saw, it seems like this could still hold the Specifications
directly, not via a pointer.

http://breakpad.appspot.com/299001/diff/8010/10004
File common/module.cc (left):

http://breakpad.appspot.com/299001/diff/8010/10004#oldcode79
Line 79: assert(!function->name.empty());
If I understand correctly, this change isn't needed any more, since we don't add
Functions to the Module until we've figured out their names.

[qsr]

Mark, is there any work done on this at the moment? I see plenty of warnings
looking like that:

the DIE at offset 0x65eb534 has a DW_AT_specification attribute referring to the
die at offset 0x65eb680, which either was not marked as a declaration, or comes
later in the file

 on binaries produced by XCode for iOS.

 Is this patch dead, or are you still working on it?

[Mark Mentovai]

I haven’t done anything with this in a while. We have the patch locally in
Chrome. respindola thinks that this may have been fixed in the debug info
produced by Clang a few months ago, but I’m not sure if that’s ever been
confirmed. The DWARF spec does allow forward references, so a change like this
one would be good even if Clang no longer needs it.

[Mark Mentovai]

I dropped this patch from Chrome in May 2013, Chrome r199286. This patch is now
totally stale, so I’m closing the Rietveld issue.