Page MenuHomePhabricator

Patch for pr17880 - main module gets base address added again to symbols on FreeBSD
AbandonedPublic

Authored by emaste on Dec 11 2013, 2:41 PM.

Details

Reviewers
jwolfe
Summary

Insure that the executable path name used in the DYLDRendezvous class can be identically matched to the executable name (if provided, as in FreeBSD) in the inferior's link_map.

Diff Detail

Event Timeline

emaste accepted this revision.Dec 17 2013, 7:17 AM

I can confirm the patch works and the approach seems reasonable. I'd like to see review from someone on a non-FreeBSD platform though.

source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
68

"git apply" complained about trailing whitespace

emaste added inline comments.Dec 17 2013, 7:19 AM
source/Commands/CommandObjectProcess.cpp
222

If we want to keep the originally provided argv0 we could instead add a new target member to store the realpath'd executable.

jwolfe updated this revision to Unknown Object (????).Jan 6 2014, 9:25 AM

Delete trailing white space reported by Ed Maste

It seems this patch slipped through the cracks, probably because we have a workaround in the version in the FreeBSD tree[1].

With the CommandObjectProcess.cpp and the change from D8296 the original warning: failed to set breakpoint site at 0x400e60 for breakpoint 2.1: failed to verify the breakpoint trap in memory. is gone, but we still add an offset to the base address. In the sample output below note that main moves from 0x00000000004010c9 to 0x00000000008010c9.

(lldb) b main
Breakpoint 1: where = dictionary`main + 25 at dictionary.c:161, address = 0x00000000004010c9
(lldb) run
Process 92032 launching
Process 92032 launched: './dictionary' (x86_64)
Process 92032 stopped
* thread #1: tid = 104044, 0x00000000008010c9 dictionary`main(argc=2, argv=0x00007fffffffe4d0) + 25 at dictionary.c:161, stop reason = breakpoint 1.1
    frame #0: 0x00000000008010c9 dictionary`main(argc=2, argv=0x00007fffffffe4d0) + 25 at dictionary.c:161
   158  int
   159  main (int argc, char **argv)
   160  {
-> 161    tree_node *dictionary = NULL;
   162    char buffer[1024];
   163    char *filename;
   164    int done = 0;
(lldb) breakpoint list
Current breakpoints:
1: name = 'main', locations = 1, resolved = 1, hit count = 1
  1.1: where = dictionary`main + 25 at dictionary.c:161, address = 0x00000000008010c9, resolved, hit count = 1

Adding @tberghammer since it seems a similar issue on Android prompted D8296.

[1] http://svnweb.freebsd.org/changeset/base/258873

source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
68–71

This part is now handled by D8296

I would prefer to keep the exe path manipulations in DYLDRendezvous only if you don't have any reason to put part of it to CommandObjectProcess.

With the current implementation I am a bit concerned what will happen in other OS-s because if one of them don't resolve the symlinks in the path stored in the SO entry then this change will break the path detection on those OS-s.

D10267 will change the way path comparison is done in DYLDRendezvous. After it is committed I think you can easily move the path resolution into DYLDRendezvous::SOEntryIsMainExecutable in the FreeBSD specific part.

I would prefer to keep the exe path manipulations in DYLDRendezvous only if you don't have any reason to put part of it to CommandObjectProcess.
...
D10267 will change the way path comparison is done in DYLDRendezvous. After it is committed I think you can easily move the path resolution into DYLDRendezvous::SOEntryIsMainExecutable in the FreeBSD specific part.

Agreed. I think building on the approach in D10267 is the way to go, although we'll still need to do have done the resolution of relative ./exe-type paths when the executable starts, not in DYLDRendezvous.

emaste commandeered this revision.Jun 5 2015, 11:25 AM
emaste edited reviewers, added: jwolfe; removed: emaste.
This revision now requires review to proceed.Jun 5 2015, 11:25 AM
emaste abandoned this revision.Jun 5 2015, 11:28 AM

A fix (for at least the common case) is in D10267. If we have an issue with time of path normalization (vs. chdir for instance) still we'll build upon that change.