Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412223008:
stack: Adjust Pre-M Android incorrect debuggerd addresses. (Closed)
|
Created:
5 years, 2 months ago by simonb (inactive) Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionstack: Adjust Pre-M Android incorrect debuggerd addresses.
Code location addresses printed by debuggerd in pre-M Android
are wrong where relocations are packed, because the libunwind
on these targets does not understand non-zero min vaddrs in
ELF LOAD segments.
Fix by searching the APK staging directory for libraries with
packed relocations, and adjusting the tombstone data on pre-
processing so that code addresses printed for these libraries
by debuggerd are offset by the min vaddr of the library that was
included in the APK.
This feature is hidden behind a new --packed-relocation-adjustments
flag, and defines a new --chrome-apk-dir flag that can be used
to indicate the location of the APK staging directory (defaults
to ../chrome_apk/ within the chrome-symbols-dir).
Uses the 'Build fingerprint' in the tombstone file to try to
auto-detect whether packed relocation adjustment should be
applied (that is, if the target is running pre-M Android).
BUG=536847
Committed: https://crrev.com/a0773c285a4a3a1a77df987a187d83096feb635f
Cr-Commit-Position: refs/heads/master@{#356858}
Committed: https://crrev.com/e06a347f46fd356f013fc6abe0af33e5ced40fab
Cr-Commit-Position: refs/heads/master@{#357104}
Patch Set 1 #Patch Set 2 : Fix options ordering issue, tidy printed file paths. #
Total comments: 16
Patch Set 3 : Make load-from-apk work. #Patch Set 4 : Update for review feedback. #Patch Set 5 : Add an attempt to auto-sense pre-M Android targets. #Patch Set 6 : Update for review comments #
Total comments: 1
Patch Set 7 : Review comment update. #
Total comments: 2
Patch Set 8 : Rebase to master. #Patch Set 9 : Fix bot breakage #
Messages
Total messages: 36 (13 generated)
simonb@chromium.org changed reviewers: + andrewhayden@chromium.org, pasko@chromium.org, primiano@chromium.org, rmcilroy@chromium.org
Another stab at a fix for the 'stack' tool to correct invalid data printed by debuggerd in pre-M Android due to packed relocations. Thoughts? Earlier WIP version here (now abandoned): https://codereview.chromium.org/1138513002 See also: b/20687795
Description was changed from ========== stack: Adjust Pre-M Android incorrect debuggerd addresses. Code location addresses printed by debuggerd in pre-M Android are wrong where relocations are packed, because the libunwind on these targets does not understand non-zero min vaddrs in ELF LOAD segments. Fix by searching the APK staging directory for libraries with packed relocations, and adjusting the tombstone data on pre- processing so that code addresses printed for these libraries by debuggerd are offset by the min vaddr of the library that was included in the APK. This feature is hidden behind a new --packed-relocation-adjustments flag, and defines a new --chrome-apk-dir flag that can be used to indicate the location of the APK staging directory (defaults to ../chrome_apk/ within the chrome-symbols-dir. Unfortunately, the 'Build fingerprint' in the tombstone file appears not to contain data stable enough to allow the stack tool to automatically identify whether packed relocation adjustment should be applied (that is, if the target is running pre-M or M Android). BUG=536847 ========== to ========== stack: Adjust Pre-M Android incorrect debuggerd addresses. Code location addresses printed by debuggerd in pre-M Android are wrong where relocations are packed, because the libunwind on these targets does not understand non-zero min vaddrs in ELF LOAD segments. Fix by searching the APK staging directory for libraries with packed relocations, and adjusting the tombstone data on pre- processing so that code addresses printed for these libraries by debuggerd are offset by the min vaddr of the library that was included in the APK. This feature is hidden behind a new --packed-relocation-adjustments flag, and defines a new --chrome-apk-dir flag that can be used to indicate the location of the APK staging directory (defaults to ../chrome_apk/ within the chrome-symbols-dir). Unfortunately, the 'Build fingerprint' in the tombstone file appears not to contain data stable enough to allow the stack tool to automatically identify whether packed relocation adjustment should be applied (that is, if the target is running pre-M or M Android). BUG=536847 ==========
https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:135: "no-packed-relocation-adjustments", Do we really need no-packed-relocation-adjustments? The default is False so we should only need the packed-relocation-adjustments flag to sets it to true. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:179: chrome_apk_dir = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) nit - just make should chrome_apk_dir be chrome_apk_path? Also, could you just set: chrome_apk_path = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) on line 150, and update it in the else block for "--chrome-apk-dir" (which should possibly also be --chrome-apk-path) ? https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_core.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_core.py:147: def _AdjustAddress(address, lib, load_vaddrs): nit - make this a private function in the PreProcessLog class https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_core.py:211: if _VALUE_LINE.match(line): nit - I know it was this way before you made your change, but since your changing this anway, could _VALUE_LINE.match(line) be added as an 'or' to the initial if above? https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_libs.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_libs.py:3: # Copyright (C) 2015 The Android Open Source Project Was this taken straight from the Android project with no modifications? If so, can we add a git hash reference in the README.chromium. Otherwise, could we land unmodified as a seperate CL (with git reference) then do the modifications in this CL. This will make it obvious what has changed if we need to cherry-pick upstream fixes.
https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:135: "no-packed-relocation-adjustments", On 2015/10/23 at 16:25:10, rmcilroy wrote: > Do we really need no-packed-relocation-adjustments? The default is False so we should only need the packed-relocation-adjustments flag to sets it to true. We don't *need* it. It's here for symmetry with the existing --more-info/--less-info, which behaves the same. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:179: chrome_apk_dir = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) On 2015/10/23 at 16:25:10, rmcilroy wrote: > nit - should chrome_apk_dir be chrome_apk_path? Again, symmetry. In this case with the existing --chrome-symbols-dir. Both are directory paths. > Also, could you just set: > chrome_apk_path = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) > on line 150, ... Not really. This isn't the correct value onto which to join any value supplied with --chrome-apk-dir (line 165). The confusion occurs because this loop may (counter-intuitively?) alter symbol.CHROME_SYMBOLS_DIR if --chrome-symbols-dir. I didn't want to disturb that existing code. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_core.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_core.py:147: def _AdjustAddress(address, lib, load_vaddrs): On 2015/10/23 at 16:25:10, rmcilroy wrote: > nit - make this a private function in the PreProcessLog class Done. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_core.py:211: if _VALUE_LINE.match(line): On 2015/10/23 at 16:25:10, rmcilroy wrote: > nit - I know it was this way before you made your change, but since your changing this anway, could _VALUE_LINE.match(line) be added as an 'or' to the initial if above? According to commentary above, we need to do the code_line match before _VALUE_LINE: 81 # Lines from 'code around' sections of the output will be matched before 82 # value lines because otheriwse the 'code around' sections will be confused as 83 # value lines. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_libs.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_libs.py:3: # Copyright (C) 2015 The Android Open Source Project On 2015/10/23 at 16:25:10, rmcilroy wrote: > Was this taken straight from the Android project with no modifications? If so, can we add a git hash reference in the README.chromium. Otherwise, could we land unmodified as a seperate CL (with git reference) then do the modifications in this CL. This will make it obvious what has changed if we need to cherry-pick upstream fixes. No, the file is brand new and only in here. Local 'modification', then. Just a bit too chunky for me to feel comfortable shoe-horning it into stack-core.py or stack itself. I've changed the copyright to Chromium style. Sorry for the wrong steer.
lgtm if comments are addressed. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:135: "no-packed-relocation-adjustments", On 2015/10/26 12:38:31, simonb wrote: > On 2015/10/23 at 16:25:10, rmcilroy wrote: > > Do we really need no-packed-relocation-adjustments? The default is False so > we should only need the packed-relocation-adjustments flag to sets it to true. > > We don't *need* it. It's here for symmetry with the existing > --more-info/--less-info, which behaves the same. I'm not convinced the symmetry adds anything, even when more/less-info has that symmetry. Unless we need it I would just remove it. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:179: chrome_apk_dir = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) On 2015/10/26 12:38:31, simonb wrote: > On 2015/10/23 at 16:25:10, rmcilroy wrote: > > nit - should chrome_apk_dir be chrome_apk_path? > > Again, symmetry. In this case with the existing --chrome-symbols-dir. Both are > directory paths. dir is fine - my confusion stemed from the fact that it looked like you were appending an APK filename, so it was no longer a directory - could you rename DEFAULT_APK to DEFAULT_APK_DIR to make this more obvious. > > > Also, could you just set: > > chrome_apk_path = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) > > on line 150, ... > > Not really. This isn't the correct value onto which to join any value supplied > with --chrome-apk-dir (line 165). > > The confusion occurs because this loop may (counter-intuitively?) alter > symbol.CHROME_SYMBOLS_DIR if --chrome-symbols-dir. I didn't want to disturb that > existing code. Ahh, modifying CHROME_SYMBOLS_DIR which looks like a constant, nice... ;). OK, in that case could you have a seperate variable for chrome-apk-dir (i.e., have it set chrome-apk-dir-suffix) and just append it to chrome_apk_dir once after the loop. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_core.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_core.py:211: if _VALUE_LINE.match(line): On 2015/10/26 12:38:31, simonb wrote: > On 2015/10/23 at 16:25:10, rmcilroy wrote: > > nit - I know it was this way before you made your change, but since your > changing this anway, could _VALUE_LINE.match(line) be added as an 'or' to the > initial if above? > > According to commentary above, we need to do the code_line match before > _VALUE_LINE: > 81 # Lines from 'code around' sections of the output will be matched before > 82 # value lines because otheriwse the 'code around' sections will be confused > as > 83 # value lines. Acknowledged. https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack_libs.py (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack_libs.py:3: # Copyright (C) 2015 The Android Open Source Project On 2015/10/26 12:38:31, simonb wrote: > On 2015/10/23 at 16:25:10, rmcilroy wrote: > > Was this taken straight from the Android project with no modifications? If so, > can we add a git hash reference in the README.chromium. Otherwise, could we land > unmodified as a seperate CL (with git reference) then do the modifications in > this CL. This will make it obvious what has changed if we need to cherry-pick > upstream fixes. > > No, the file is brand new and only in here. Local 'modification', then. Just a > bit too chunky for me to feel comfortable shoe-horning it into stack-core.py or > stack itself. > > I've changed the copyright to Chromium style. Sorry for the wrong steer. Acknowledged.
Description was changed from ========== stack: Adjust Pre-M Android incorrect debuggerd addresses. Code location addresses printed by debuggerd in pre-M Android are wrong where relocations are packed, because the libunwind on these targets does not understand non-zero min vaddrs in ELF LOAD segments. Fix by searching the APK staging directory for libraries with packed relocations, and adjusting the tombstone data on pre- processing so that code addresses printed for these libraries by debuggerd are offset by the min vaddr of the library that was included in the APK. This feature is hidden behind a new --packed-relocation-adjustments flag, and defines a new --chrome-apk-dir flag that can be used to indicate the location of the APK staging directory (defaults to ../chrome_apk/ within the chrome-symbols-dir). Unfortunately, the 'Build fingerprint' in the tombstone file appears not to contain data stable enough to allow the stack tool to automatically identify whether packed relocation adjustment should be applied (that is, if the target is running pre-M or M Android). BUG=536847 ========== to ========== stack: Adjust Pre-M Android incorrect debuggerd addresses. Code location addresses printed by debuggerd in pre-M Android are wrong where relocations are packed, because the libunwind on these targets does not understand non-zero min vaddrs in ELF LOAD segments. Fix by searching the APK staging directory for libraries with packed relocations, and adjusting the tombstone data on pre- processing so that code addresses printed for these libraries by debuggerd are offset by the min vaddr of the library that was included in the APK. This feature is hidden behind a new --packed-relocation-adjustments flag, and defines a new --chrome-apk-dir flag that can be used to indicate the location of the APK staging directory (defaults to ../chrome_apk/ within the chrome-symbols-dir). Uses the 'Build fingerprint' in the tombstone file to try to auto-detect whether packed relocation adjustment should be applied (that is, if the target is running pre-M Android). BUG=536847 ==========
https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:135: "no-packed-relocation-adjustments", On 2015/10/26 at 16:42:17, rmcilroy wrote: > I'm not convinced the symmetry adds anything, even when more/less-info has that symmetry. Unless we need it I would just remove it. Now useful as a way to override any state that may be incorrectly auto-detected from parsing the build fingerprint (introduced in patch set 4). https://codereview.chromium.org/1412223008/diff/20001/third_party/android_pla... third_party/android_platform/development/scripts/stack:179: chrome_apk_dir = os.path.join(symbol.CHROME_SYMBOLS_DIR, '..', DEFAULT_APK) On 2015/10/26 at 16:42:17, rmcilroy wrote: > ... > dir is fine - my confusion stemed from the fact that it looked like you were appending an APK filename, so it was no longer a directory - could you rename DEFAULT_APK to DEFAULT_APK_DIR to make this more obvious. Done
Ross, I added some more code since prior lgtm. Still good, or more feedback? Thanks.
Still lgtm, thanks https://codereview.chromium.org/1412223008/diff/100001/third_party/android_pl... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/100001/third_party/android_pl... third_party/android_platform/development/scripts/stack:182: '..', DEFAULT_APK_DIR) You've missed this comment from the previous patchset: "In that case could you have a seperate variable for chrome-apk-dir (i.e., have it set chrome-apk-dir-suffix) and just append it to chrome_apk_dir once after the loop."
On 2015/10/28 at 17:32:33, rmcilroy wrote: > Still lgtm, thanks > ... > > "In that case could you have a seperate variable for chrome-apk-dir (i.e., have > it set chrome-apk-dir-suffix) and just append it to chrome_apk_dir once after > the loop." I couldn't convince myself it was helpful. Because we want to append to the pre-loop value of (non-constant) symbol.CHROME_SYMBOLS_DIR if --chrome-apk-dir is given, we need two variables either way. Anyway, done. See what you think.
LGTM! Thanks especially for trying to keep the library logic separate and for the useful examples in the comments there, it really helps with understanding. https://codereview.chromium.org/1412223008/diff/120001/third_party/android_pl... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/120001/third_party/android_pl... third_party/android_platform/development/scripts/stack:193: print "Searching for native crashes in: " + os.path.realpath(arguments[0]) Nit only: Just out of curiosity, why convert this from "%s" to ":" ?
still LGTM, thanks.
https://codereview.chromium.org/1412223008/diff/120001/third_party/android_pl... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1412223008/diff/120001/third_party/android_pl... third_party/android_platform/development/scripts/stack:193: print "Searching for native crashes in: " + os.path.realpath(arguments[0]) On 2015/10/29 at 07:44:35, Andrew Hayden (chromium.org) wrote: > Nit only: Just out of curiosity, why convert this from "%s" to ":" ? Matches the way the other trace-like messages are formatted in this module.
The CQ bit was checked by simonb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412223008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412223008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by simonb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from andrewhayden@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1412223008/#ps140001 (title: "Rebase to master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412223008/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412223008/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a0773c285a4a3a1a77df987a187d83096feb635f Cr-Commit-Position: refs/heads/master@{#356858}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1407143010/ by simonb@chromium.org. The reason for reverting is: Breaks bot. Example: http://build.chromium.org/p/chromium.android/builders/Android%20Aura%20Tester... .
I know you've reverted this, but just add this failure log FYI: http://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/31805... http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/23... http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
The CQ bit was checked by simonb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412223008/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412223008/150001
The CQ bit was unchecked by simonb@chromium.org
The CQ bit was checked by simonb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from andrewhayden@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1412223008/#ps150001 (title: "Fix bot breakage")
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by simonb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412223008/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412223008/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e06a347f46fd356f013fc6abe0af33e5ced40fab Cr-Commit-Position: refs/heads/master@{#357104} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
