Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(415)

Issue 2730473002: Build addr2line-pdb from tcmalloc for use in memory-infra symbolization (Closed)

Created:
3 years, 9 months ago by ajwong
Modified:
3 years, 9 months ago
CC:
chromium-reviews, Dai Mikurube (NOT FULLTIME)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Build addr2line-pdb from tcmalloc for use in memory-infra symbolization The memory-infra tools were first written on linux where the symbolization was done using addr2line. Turns out tcmalloc has some old code (with a few small bugs) that wraps the windows dbghelp.dll symbolization APIs in an addr2line-like interface called addr2line-pdb. This CL * fixes the small format string bugs * makes the windows library configuration #defines more friendly for chromium build * modifies addr2line to expect addresses relative to DllBase to better simulate how addr2line works with modules in linux. The last point is important. Windows DLL have a concept of "default load address" which hints to the OS where to load the binary image after relocation. The dbghelp.dll symbolization library will load the module at this location in the virtual address space meaning the caller of these functions would need to be aware of the base address. This makes things unnecessarily complex in the face of ASLR and also diverges from the behavior of addr2line when used with linux-style DSOs. This CL simply adds the module base address to the incoming addresses thereby making the input relative addresses for the module which both is easier to use and lines up better with linux's addr2line behavior. BUG=694792 Review-Url: https://codereview.chromium.org/2730473002 Cr-Commit-Position: refs/heads/master@{#457271} Committed: https://chromium.googlesource.com/chromium/src/+/bf193279edce86f900ae5b222dc93ea427ae1ff0

Patch Set 1 #

Patch Set 2 : make it all work #

Total comments: 4

Patch Set 3 : address comments #

Patch Set 4 : reformated #

Total comments: 3

Patch Set 5 : move GN target #

Patch Set 6 : Move target correctly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -8 lines) Patch
M BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
A third_party/tcmalloc/BUILD.gn View 1 2 1 chunk +14 lines, -0 lines 0 comments Download
M third_party/tcmalloc/README.chromium View 1 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/tcmalloc/chromium/src/windows/addr2line-pdb.c View 1 7 chunks +25 lines, -8 lines 0 comments Download

Messages

Total messages: 46 (25 generated)
awong
3 years, 9 months ago (2017-03-01 22:29:29 UTC) #6
erikchen
Thanks, lgtm with nits. Please wrap your CL description to 80 columns. I wasn't familiar ...
3 years, 9 months ago (2017-03-01 22:42:51 UTC) #7
awong
comment wrapped. https://codereview.chromium.org/2730473002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2730473002/diff/20001/BUILD.gn#newcode676 BUILD.gn:676: "//third_party/tcmalloc:addr2line-pdb", On 2017/03/01 22:42:51, erikchen wrote: > ...
3 years, 9 months ago (2017-03-01 22:57:01 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2730473002/60001
3 years, 9 months ago (2017-03-02 21:26:39 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/377231)
3 years, 9 months ago (2017-03-02 21:35:03 UTC) #19
awong
Heya will...i can has an OWNERS review?
3 years, 9 months ago (2017-03-02 22:52:28 UTC) #21
awong
Heya will...i can has an OWNERS review?
3 years, 9 months ago (2017-03-02 22:52:32 UTC) #22
Will Harris
On 2017/03/02 22:52:32, awong wrote: > Heya will...i can has an OWNERS review? hmm - ...
3 years, 9 months ago (2017-03-02 23:02:33 UTC) #23
awong
On 2017/03/02 23:02:33, Will Harris wrote: > On 2017/03/02 22:52:32, awong wrote: > > Heya ...
3 years, 9 months ago (2017-03-02 23:06:54 UTC) #24
Will Harris
okay I trust your judgement so lgtm
3 years, 9 months ago (2017-03-02 23:08:28 UTC) #26
awong
On 2017/03/02 23:08:28, Will Harris wrote: > okay I trust your judgement so lgtm <3
3 years, 9 months ago (2017-03-02 23:09:29 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2730473002/60001
3 years, 9 months ago (2017-03-02 23:10:11 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/377344)
3 years, 9 months ago (2017-03-02 23:21:11 UTC) #31
scottmg
lgtm for BUILD.gn after moving. As far as general approach, either way seems fine, but ...
3 years, 9 months ago (2017-03-03 00:20:24 UTC) #33
awong
https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn#newcode758 BUILD.gn:758: deps += [ "//third_party/tcmalloc:addr2line-pdb" ] On 2017/03/03 00:20:24, scottmg ...
3 years, 9 months ago (2017-03-06 18:45:52 UTC) #34
erikchen
https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn#newcode758 BUILD.gn:758: deps += [ "//third_party/tcmalloc:addr2line-pdb" ] On 2017/03/06 18:45:52, awong ...
3 years, 9 months ago (2017-03-06 19:31:18 UTC) #35
scottmg
On 2017/03/06 19:31:18, erikchen wrote: > https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn > File BUILD.gn (right): > > https://codereview.chromium.org/2730473002/diff/60001/BUILD.gn#newcode758 > ...
3 years, 9 months ago (2017-03-06 19:37:40 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2730473002/100001
3 years, 9 months ago (2017-03-14 21:46:00 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/383452)
3 years, 9 months ago (2017-03-14 23:25:11 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2730473002/100001
3 years, 9 months ago (2017-03-15 22:33:37 UTC) #43
commit-bot: I haz the power
3 years, 9 months ago (2017-03-16 00:01:33 UTC) #46
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/bf193279edce86f900ae5b222dc9...

Powered by Google App Engine
This is Rietveld 408576698