Use DW_AT_MIPS_linkage_name if it is available.

Use DW_AT_MIPS_linkage_name if it is available.

[Ted Mielczarek]

This patch LGTM except for some minor formating issues, but I think it needs a
unit test. There's a bunch of tests for this code in
dwarf_cu_to_module_unittest.cc, it shouldn't be too hard to copy the OneFunc
test and extend it to test this attribute as well. You'd probably want to copy
the CUFixtureBase::DefineFunction method and make a
DefineFunctionWithLinkageName or something like that.

https://breakpad.appspot.com/457002/diff/1/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/457002/diff/1/src/common/dwarf_cu_to_module.cc#n...
src/common/dwarf_cu_to_module.cc:227: // assumption about how strings are
implemented?
These comments should be fixed to use third-person instead of first-person while
you're here. (If you'd like I can do the comment fixup when I land this.)

https://breakpad.appspot.com/457002/diff/1/src/common/dwarf_cu_to_module.cc#n...
src/common/dwarf_cu_to_module.cc:305: const char *demangled =
abi::__cxa_demangle(data.c_str(), NULL, NULL, NULL);
The GCC docs say __cxa_demangle just returns char*, which should make jumping
through hoops to cast it below unnecessary:
http://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.3/a01696.html

[Ted Mielczarek]

Aside from splitting out the unrelated change and a few minor nits this looks
good. Get me an updated patch and I can land it.

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf/dwarf2diehandl...
File src/common/dwarf/dwarf2diehandler.h (left):

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf/dwarf2diehandl...
src/common/dwarf/dwarf2diehandler.h:247: const AttributeList &attrs) {
I think this should be a separate patch since it's unrelated.

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module.c...
src/common/dwarf_cu_to_module.cc:224: // counting internally, so the effect is
to have all our data structures
"our" should just be "the".

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module.c...
src/common/dwarf_cu_to_module.cc:244: // string if the DIE has no such attribute
or we failed to demangle its
remove the "we". You could say "or its content could not be demangled."

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module.c...
src/common/dwarf_cu_to_module.cc:305: char *demangled =
abi::__cxa_demangle(data.c_str(), NULL, NULL, NULL);
nit: char* (* next to type name)

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module.c...
src/common/dwarf_cu_to_module.cc:318: // so we don't need to add enclosing_name.
Remove the "we". Perhaps "It is already qualified, so there's no need to add
enclosing_name."

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module_u...
File src/common/dwarf_cu_to_module_unittest.cc (right):

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module_u...
src/common/dwarf_cu_to_module_unittest.cc:206: const char* mangled_name = NULL);
Google's style guide says not to use default arguments. It's kind of a pain, but
can you leave off the =NULL and just append NULL to the argument list of all the
DefineFunction calls in this file?

https://breakpad.appspot.com/457002/diff/4001/src/common/dwarf_cu_to_module_u...
src/common/dwarf_cu_to_module_unittest.cc:718: TestFunction(0, "n::f(int)",
0x938cf8c07def4d34ULL, 0x55592d727f6cd01fLL);
Thanks for adding a test!

[Ted Mielczarek]

LGTM. I'll land this for you.

[Ryan Sleevi]

Random drive-by about a comment that I think is functionally and factually
problematic.

https://breakpad.appspot.com/457002/diff/11001/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/457002/diff/11001/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:227: // assumption about how strings are
implemented?
As a drive by, I think at this point, only libstdc++ is using ref-counted
strings. Both MSVC and libc++ do not, and many projects I've worked on tend to
disable libstdc++'s refcounting due to contention issues.

Definitely would encourage you to not rely on these implementation details,
since the move with C++11 is to not use CoW at all, for the issues it can cause
(with .c_str()/.data() mutation in particular, which effectively "everybody"
does)

[rafael.espindola]

> src/common/dwarf_cu_to_module.cc:227: // assumption about how strings
> are implemented?
> As a drive by, I think at this point, only libstdc++ is using
> ref-counted strings. Both MSVC and libc++ do not, and many projects I've
> worked on tend to disable libstdc++'s refcounting due to contention
> issues.
>
> Definitely would encourage you to not rely on these implementation
> details, since the move with C++11 is to not use CoW at all, for the
> issues it can cause (with .c_str()/.data() mutation in particular, which
> effectively "everybody" does)

Ah. I knew about libstc++ and libc++, but believed the comment was
accurate for msvc. Note that that part of the comment (and the
assumption) was already in the code. I just factored it and added the
suggestion of using a string_ref.

Cheers,
Rafael

[jimb]

Well, to be clear, we don't *rely* on strings being reference-counted; the code
is correct no matter how std::string is implemented. But on systems that do
reference count strings, that code saves a drastic amount of memory.

So, is it the case that on non-libstdc++ systems, assigning strings always
entails copying, and it is much more efficient to pass strings by reference? If
so, that's too bad; switching to pointers and references instead of simply
assigning strings will take a lot more bookkeeping.

[jimb]

On 2012/10/05 17:57:58, Ryan Sleevi wrote:
> Random drive-by about a comment that I think is functionally and factually
> problematic.
> 
>
https://breakpad.appspot.com/457002/diff/11001/src/common/dwarf_cu_to_module.cc
> File src/common/dwarf_cu_to_module.cc (right):
> 
>
https://breakpad.appspot.com/457002/diff/11001/src/common/dwarf_cu_to_module....
> src/common/dwarf_cu_to_module.cc:227: // assumption about how strings are
> implemented?
> As a drive by, I think at this point, only libstdc++ is using ref-counted
> strings. Both MSVC and libc++ do not, and many projects I've worked on tend to
> disable libstdc++'s refcounting due to contention issues.

If the comment is wrong now, it definitely should be changed.

[rafael.espindola]

> So, is it the case that on non-libstdc++ systems, assigning strings
> always entails copying, and it is much more efficient to pass strings by
> reference? If so, that's too bad; switching to pointers and references
> instead of simply assigning strings will take a lot more bookkeeping.

The string_ref in the comment is:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html

it is a more generic/easier to use version of "const std::string &".
It is used a lot in llvm (where it is called StringRef. I am not sure
how many strings are used in breakdpad (and in how many places) to
know if it would be worth the cost.

Cheers,
Rafael

[rafael.espindola]

> If the comment is wrong now, it definitely should be changed.

I will update it.

Cheers,
Rafael

[jimb_red-bean.com]

On Fri, Oct 5, 2012 at 1:45 PM, Rafael Espíndola
<rafael.espindola@gmail.com> wrote:
>> If the comment is wrong now, it definitely should be changed.
>
> I will update it.

Ah, thanks!

[jimb_red-bean.com]

On Fri, Oct 5, 2012 at 1:44 PM, Rafael Espíndola
<rafael.espindola@gmail.com> wrote:
> The string_ref in the comment is:
>
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html
>
> it is a more generic/easier to use version of "const std::string &".
> It is used a lot in llvm (where it is called StringRef. I am not sure
> how many strings are used in breakdpad (and in how many places) to
> know if it would be worth the cost.

Well, our concern isn't so much about being able to access characters
without regard to the type that owns them, which is what string_ref
seems to be about. It'd be fine for us to just use something like
shared_ptr<string>.

[jimb_red-bean.com]

On Fri, Oct 5, 2012 at 6:19 PM, Jim Blandy <jimb@red-bean.com> wrote:
> Well, our concern isn't so much about being able to access characters
> without regard to the type that owns them, which is what string_ref
> seems to be about. It'd be fine for us to just use something like
> shared_ptr<string>.

Rather, shared_ptr<string> would be *preferable* for us, because it
lets us share the same string between a bunch of objects whose
lifetimes we don't really want to have to think about too carefully.

[rafael.espindola]

> Rather, shared_ptr<string> would be *preferable* for us, because it
> lets us share the same string between a bunch of objects whose
> lifetimes we don't really want to have to think about too carefully.

The existing comment suggest that the central map where we add the
strings outlives every use. Is that not the case?

Cheers,
Rafael

[jimb_red-bean.com]

On Sat, Oct 6, 2012 at 5:49 AM, Rafael Espíndola
<rafael.espindola@gmail.com> wrote:
>> Rather, shared_ptr<string> would be *preferable* for us, because it
>> lets us share the same string between a bunch of objects whose
>> lifetimes we don't really want to have to think about too carefully.
>
> The existing comment suggest that the central map where we add the
> strings outlives every use. Is that not the case?

Actually, yes --- and that's it's purpose. So we could use std::string &.