Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(9)

Issue 7774002: Handle ARM THUMB functions when removing duplicate PUBLIC entries. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 5 months ago by Lei Zhang (chromium)
Modified:
6 years, 5 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Handle ARM THUMB functions when removing duplicate PUBLIC entries.

In ELF symtab/dynsym sections, THUMB function addresses have bit 0 set,
whereas the DWARF function entries are not.

R=mark@chromium.org

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : check bit 0 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M src/common/module.cc View 1 1 chunk +18 lines, -3 lines 2 comments Download

Messages

Total messages: 8
Lei Zhang (chromium)
Follow up to yesterday's CL: https://breakpad.appspot.com/2844002/
6 years, 5 months ago #1
Mark Mentovai
https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode88 src/common/module.cc:88: // ARM THUMB functions are offset by 1 byte. ...
6 years, 5 months ago #2
Lei Zhang (chromium)
https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode88 src/common/module.cc:88: // ARM THUMB functions are offset by 1 byte. ...
6 years, 5 months ago #3
Mark Mentovai
LGTM https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode90 src/common/module.cc:90: // ARM THUMB functions have bit 0 set. ...
6 years, 5 months ago #4
Lei Zhang (chromium)
Committed patchset #2 (id:40001) manually as r1423 (presubmit successful).
6 years, 5 months ago #5
hashimoto
https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126 src/common/module.cc:126: void Module::AddExtern(Extern *ext) { How about clearing LSB of ...
6 years, 5 months ago #6
Lei Zhang (chromium)
On 2015/02/04 10:42:46, hashimoto wrote: > https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc > File src/common/module.cc (right): > > https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126 > ...
6 years, 5 months ago #7
hashimoto
6 years, 5 months ago #8
Message was sent while issue was closed.
On 2015/02/05 23:49:02, Lei Zhang (chromium) wrote:
> On 2015/02/04 10:42:46, hashimoto wrote:
> > https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc
> > File src/common/module.cc (right):
> > 
> >
>
https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126
> > src/common/module.cc:126: void Module::AddExtern(Extern *ext) {
> > How about clearing LSB of Extern's address instead? (here, or at the caller
> > site)
> > 
> > minidump_stackwalk uses information from PUBLIC lines as the last resort
> option
> > to look up the function name.
> >
>
(https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/b...)
> > As the current code may output PUBLIC lines with offset +1, IIUC
> > minidump_stackwalk may display a wrong function name if the address is at
the
> > start of the function.
> 
> Is https://breakpad.appspot.com/2854003 what you are thinking of?

Thanks, yes, that's what I was thinking.

Or, as AddExtern() is called by only two places (stabs_reader.cc and
elf_symbols_to_module.cc), it can be addressed in the caller site too.
elf_symbols_to_module.cc can clear the lsb of the address for ARM before calling
AddExtern().
I don't know much about stabs_reader.cc, but as what .stab contains is debug
info, I think we don't have to do anything to address values stored in stab
(i.e. I guess an address stored in .stab section is the actual PC value, not the
value decorated with Thumb information in LSB).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted