Refactor .so name detection logic in minidump/linux_dumper.

Refactor .so name detection logic in minidump/linux_dumper.

This is a refactoring of the logic which determines the
module name and path for a given MappingInfo in minidump_writer.cc.
Such logic, which will be soon shared also with the upcoming
microdump_writer.cc, is simply being moved to linux_dumper.cc,
extracting a GetMappingEffectiveNameAndPath method.
No behavioral change is intended.

BUG=chromium:410294
R=thestig@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1392

[primiano]

thestig@, can I ask you to take a look to this CL? 

You seem to have recently reviewed some of these lines which anton@ wrote* 
You've done that so nicely that you won another round for free (this must be
your lucky day) ;-D

mmandlis@ can I ask you to take an eye as well to see if everything sounds
reasonable?

* Unforunatelly anton@ is on vacation till next week.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:96:
LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
This const reference is a lie (see below)

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:137: sizeof(kDeletedSuffix) +
1] = '\0';
This line of code has caused me ~1h of headache and has bumped my grumpiness
level from "moderately everyday cynical" to "skyrocket over the stars":
1. Why a method which is called ElfFileIdentifierForMapping mangle the path of
the mapping argument without mentioning or even a comment on that?
2. Why this method changes something from a member which is passed as a const
reference? :°°(

If there are no objections I plan to update this interface (pass a ptr instead
of a const ref and reword a bit the name) in a separate CL (there is stuff  /
tests depending on this and doing it in this CL would make rolling / reverting
more complicating)

[Lei Zhang (chromium)]

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:96:
LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
On 2014/10/14 17:03:39, primiano wrote:
> This const reference is a lie (see below)

git/svn blame can tell you who did this. I hope it's not me. :)

In any case, it does seem redundant to pass in |mapping|, since one can get to
it easily via |mapping_id|.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:227: file_path_avail_len -=
*file_path_len;
If I read the strlcat() man page correctly, you don't need to do this. Same for
line 247 below.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.h (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.h:137: // |file_path_len| and
|file_name_len| are bidirectional arguments: they pass
Do these have to be in/out parameters? Can they be just in-parameters? If a
caller needs to know the string length, they can always call my_strlen()
themselves.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/minidump_writer.cc:556: // Note:
ElfFileIdentifierForMapping can manipulate the mapping.name.
nit: "Note: ElfFileIdentifierForMapping() can manipulate |mapping.name|."

[primiano]

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:96:
LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
On 2014/10/14 22:38:02, Lei Zhang (chromium) wrote:
> On 2014/10/14 17:03:39, primiano wrote:
> > This const reference is a lie (see below)
> 
> git/svn blame can tell you who did this. I hope it's not me. :)
> 
> In any case, it does seem redundant to pass in |mapping|, since one can get to
> it easily via |mapping_id|.

Yeah, I didn't even realize. I will fix this in the next refactor CL.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:227: file_path_avail_len -=
*file_path_len;
On 2014/10/14 22:38:02, Lei Zhang (chromium) wrote:
> If I read the strlcat() man page correctly, you don't need to do this. Same
for
> line 247 below.

Oh right. I also removed the extra soname copy at this point.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.h (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.h:137: // |file_path_len| and
|file_name_len| are bidirectional arguments: they pass
On 2014/10/14 22:38:02, Lei Zhang (chromium) wrote:
> Do these have to be in/out parameters? Can they be just in-parameters? If a
> caller needs to know the string length, they can always call my_strlen()
> themselves.
Yeah makes sense. Also the API looks cleaner. Changed.

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/minidump_writer.cc:556: // Note:
ElfFileIdentifierForMapping can manipulate the mapping.name.
On 2014/10/14 22:38:02, Lei Zhang (chromium) wrote:
> nit: "Note: ElfFileIdentifierForMapping() can manipulate |mapping.name|."

Done.

[primiano]

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7734002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_dumper.cc:96:
LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
On 2014/10/15 08:43:06, primiano wrote:
> On 2014/10/14 22:38:02, Lei Zhang (chromium) wrote:
> > On 2014/10/14 17:03:39, primiano wrote:
> > > This const reference is a lie (see below)
> > 
> > git/svn blame can tell you who did this. I hope it's not me. :)
> > 
> > In any case, it does seem redundant to pass in |mapping|, since one can get
to
> > it easily via |mapping_id|.
> 
> Yeah, I didn't even realize. I will fix this in the next refactor CL.

Actually turns out that ElfFileIdentifierForMapping is more arcane than I
thought. Conversely to what we thought, |mapping| is NOT redundant here.
|mapping| is not necessarily lookup-able by mapping_id, hence not necessarily
part of |mappings_| (when member = false), that's the reason why we need both.
The CL that introduced this was: https://breakpad.appspot.com/242001/show

To be honest, IMHO this code here is a bit gross:
HandleDeletedFileInMapping() modifies the |filename| to remove "(deleted)" from
the file path when needed (its argument is bidirectional).
However, we never look into the out |path| returned by
HandleDeletedFileInMapping(), rather this function here just speculates on its
implementation: when filename_modified == true, we assume it did truncate the
path of exactly (sizeof(kDeletedSuffix) + 1) characters. Hence, the code below
(~line 136) adds a null-terminator on the basis of such speculation @
filename_len -  sizeof(kDeletedSuffix) + 1 .
Having said this, I don't see at this point a way to resolve this grossness
which doesn't involve rewriting half of this file, which, to be honest, I'd
prefer not to do considering the wide number of projects depending on this and
the poor test coverage.

[Lei Zhang (chromium)]

lgtm

Sigh, please file a google-breakpad bug so we can clean this up later.

https://breakpad.appspot.com/7734002/diff/100001/src/client/linux/minidump_wr...
File src/client/linux/minidump_writer/linux_dumper.h (left):

https://breakpad.appspot.com/7734002/diff/100001/src/client/linux/minidump_wr...
src/client/linux/minidump_writer/linux_dumper.h:120: // Find the shared object
name (SONAME) by examining the ELF information
Can you move this comment into linux_dumper.cc?

[primiano]

On 2014/10/15 19:00:10, Lei Zhang (chromium) wrote:
> lgtm
Thanks

 
> Sigh, please file a google-breakpad bug so we can clean this up later.

https://code.google.com/p/google-breakpad/issues/detail?id=611

> Can you move this comment into linux_dumper.cc?
Done

[primiano]

Committed patchset #4 (id:180001) manually as 1392 (presubmit successful).

[Ted Mielczarek]

On 2014/10/15 19:00:10, Lei Zhang (chromium) wrote:
> lgtm
> 
> Sigh, please file a google-breakpad bug so we can clean this up later.

Sorry, this is probably all my fault. We're using the MappingInfo stuff for our
Firefox Android builds where we have our own dynamic linker as well. My coworker
has a set of Breakpad patches that have been bitrotting because I never got
around to reviewing them that should let us remove all the MappingInfo junk:
https://breakpad.appspot.com/580002/
https://breakpad.appspot.com/573002/
https://breakpad.appspot.com/562002/
https://breakpad.appspot.com/571003/
https://breakpad.appspot.com/578002/
https://breakpad.appspot.com/587002/

(I think that's all of them.) If one of you has bandwidth to review those we
could clean a lot of this up.