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

Issue 2559323007: Improve EAT_STREAM_PARAMETERS for Windows x86 (Closed)

Created:
4 years ago by scottmg
Modified:
4 years ago
Reviewers:
dcheng
CC:
chromium-reviews, vmpstr+watch_chromium.org, jschuh, Primiano Tucci (use gerrit)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Improve EAT_STREAM_PARAMETERS for Windows x86 Dumps of check_example.exe Current: ?DoBlinkReleaseAssert@@YAX_N@Z: 00404EDC: 55 push ebp 00404EDD: 8B EC mov ebp,esp 00404EDF: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EE3: 75 07 jne 00404EEC 00404EE5: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404EEC: 5D pop ebp 00404EED: C3 ret ?DoCheck@@YAX_N@Z: 00404EEE: 55 push ebp 00404EEF: 8B EC mov ebp,esp 00404EF1: 51 push ecx 00404EF2: 83 65 FC 00 and dword ptr [ebp-4],0 00404EF6: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EFA: 75 07 jne 00404F03 00404EFC: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404F03: 8B E5 mov esp,ebp 00404F05: 5D pop ebp 00404F06: C3 ret _main: 00404F07: 55 push ebp 00404F08: 8B EC mov ebp,esp 00404F0A: 83 7D 08 02 cmp dword ptr [ebp+8],2 00404F0E: 53 push ebx 00404F0F: 0F 9F C3 setg bl 00404F12: 53 push ebx 00404F13: E8 D6 FF FF FF call ?DoCheck@@YAX_N@Z 00404F18: 53 push ebx 00404F19: E8 BE FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404F1E: 59 pop ecx 00404F1F: 59 pop ecx 00404F20: 33 C0 xor eax,eax 00404F22: 5B pop ebx 00404F23: 5D pop ebp 00404F24: C3 ret After this CL: ?DoBlinkReleaseAssert@@YAX_N@Z: 00404EAC: 55 push ebp 00404EAD: 8B EC mov ebp,esp 00404EAF: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EB3: 75 07 jne 00404EBC 00404EB5: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404EBC: 5D pop ebp 00404EBD: C3 ret _main: 00404EBE: 55 push ebp 00404EBF: 8B EC mov ebp,esp 00404EC1: 83 7D 08 02 cmp dword ptr [ebp+8],2 00404EC5: 53 push ebx 00404EC6: 0F 9F C3 setg bl 00404EC9: 53 push ebx 00404ECA: E8 DD FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404ECF: 53 push ebx 00404ED0: E8 D7 FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404ED5: 59 pop ecx 00404ED6: 59 pop ecx 00404ED7: 33 C0 xor eax,eax 00404ED9: 5B pop ebx 00404EDA: 5D pop ebp 00404EDB: C3 ret Amusingly, I was confused because I thought I was going crazy when DoCheck wasn't showing up in the /disasm. But of course, it's because it got COMDAT'd with the Blink one, as we want. :) R=primiano@chromium.org BUG=672699 Review-Url: https://codereview.chromium.org/2559323007 Committed: https://crrev.com/3c957a583f692a71024bddbcf50b39a69af89d6e Cr-Commit-Position: refs/heads/master@{#437777}

Patch Set 1 #

Patch Set 2 : tidy #

Patch Set 3 : voidify #

Patch Set 4 : whiny gcc #

Patch Set 5 : more whinery #

Patch Set 6 : parens #

Total comments: 2

Patch Set 7 : comment #

Total comments: 4

Patch Set 8 : nullptr #

Patch Set 9 : 0 #

Total comments: 2

Patch Set 10 : nit #

Patch Set 11 : back to ps7, extern ostream* #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -4 lines) Patch
M base/check_example.cc View 1 2 chunks +13 lines, -1 line 0 comments Download
M base/logging.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +17 lines, -3 lines 0 comments Download
M base/logging.cc View 1 2 3 4 5 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 84 (54 generated)
dcheng
Seems reasonable to me, let's see how the trybots like it.
4 years ago (2016-12-09 18:16:56 UTC) #5
scottmg
For reference, primiano came up with another approach we discussed offline that we could try ...
4 years ago (2016-12-09 19:04:21 UTC) #10
scottmg
On 2016/12/09 19:04:21, scottmg wrote: > For reference, primiano came up with another approach we ...
4 years ago (2016-12-09 19:05:25 UTC) #11
dcheng
On 2016/12/09 19:05:25, scottmg wrote: > On 2016/12/09 19:04:21, scottmg wrote: > > For reference, ...
4 years ago (2016-12-09 19:12:53 UTC) #12
scottmg
On 2016/12/09 19:12:53, dcheng wrote: > On 2016/12/09 19:05:25, scottmg wrote: > > On 2016/12/09 ...
4 years ago (2016-12-09 22:37:40 UTC) #28
dcheng
https://codereview.chromium.org/2559323007/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/160001/base/logging.h#newcode431 base/logging.h:431: #define EAT_STREAM_PARAMETERS \ Can we add a comment that ...
4 years ago (2016-12-09 23:16:48 UTC) #31
scottmg
https://codereview.chromium.org/2559323007/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/160001/base/logging.h#newcode431 base/logging.h:431: #define EAT_STREAM_PARAMETERS \ On 2016/12/09 23:16:48, dcheng wrote: > ...
4 years ago (2016-12-09 23:30:27 UTC) #32
dcheng
On 2016/12/09 23:30:27, scottmg wrote: > https://codereview.chromium.org/2559323007/diff/160001/base/logging.h > File base/logging.h (right): > > https://codereview.chromium.org/2559323007/diff/160001/base/logging.h#newcode431 > ...
4 years ago (2016-12-09 23:35:18 UTC) #33
scottmg
On 2016/12/09 23:35:18, dcheng wrote: > On 2016/12/09 23:30:27, scottmg wrote: > > https://codereview.chromium.org/2559323007/diff/160001/base/logging.h > ...
4 years ago (2016-12-09 23:41:53 UTC) #36
scottmg
On 2016/12/09 23:41:53, scottmg wrote: > On 2016/12/09 23:35:18, dcheng wrote: > > On 2016/12/09 ...
4 years ago (2016-12-09 23:46:41 UTC) #37
dcheng
https://codereview.chromium.org/2559323007/diff/180001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/180001/base/logging.h#newcode431 base/logging.h:431: // Note that g_swallow_stream is used instead of an ...
4 years ago (2016-12-10 00:39:33 UTC) #38
scottmg
https://codereview.chromium.org/2559323007/diff/180001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/180001/base/logging.h#newcode431 base/logging.h:431: // Note that g_swallow_stream is used instead of an ...
4 years ago (2016-12-10 00:57:31 UTC) #41
dcheng
LGTM https://codereview.chromium.org/2559323007/diff/220001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/220001/base/logging.h#newcode432 base/logging.h:432: // pointless instructions to be emitted even at ...
4 years ago (2016-12-10 03:44:03 UTC) #48
scottmg
https://codereview.chromium.org/2559323007/diff/220001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2559323007/diff/220001/base/logging.h#newcode432 base/logging.h:432: // pointless instructions to be emitted even at full ...
4 years ago (2016-12-10 05:02:01 UTC) #49
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/2559323007/240001
4 years ago (2016-12-10 05:02:31 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/196523)
4 years ago (2016-12-10 06:32:20 UTC) #54
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/2559323007/240001
4 years ago (2016-12-10 06:42:26 UTC) #56
commit-bot: I haz the power
Committed patchset #10 (id:240001)
4 years ago (2016-12-10 08:30:17 UTC) #59
findit-for-me
FYI: Findit identified this CL at revision 437763 as the culprit for failures in the ...
4 years ago (2016-12-10 08:52:52 UTC) #60
Primiano Tucci (use gerrit)
Ahhh I think we need to be slightly smarter to trick compilers that run with ...
4 years ago (2016-12-10 10:28:35 UTC) #61
Primiano Tucci (use gerrit)
A revert of this CL (patchset #10 id:240001) has been created in https://codereview.chromium.org/2569583002/ by primiano@chromium.org. ...
4 years ago (2016-12-10 10:29:40 UTC) #62
scottmg
On 2016/12/10 10:28:35, Primiano Tucci wrote: > Ahhh I think we need to be slightly ...
4 years ago (2016-12-10 15:18:21 UTC) #64
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/2559323007/280001
4 years ago (2016-12-10 15:50:38 UTC) #71
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/196596)
4 years ago (2016-12-10 17:08:56 UTC) #73
dcheng
Sigh. Still LGTM
4 years ago (2016-12-10 19:51:30 UTC) #74
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/2559323007/280001
4 years ago (2016-12-10 19:51:53 UTC) #76
commit-bot: I haz the power
Committed patchset #11 (id:280001)
4 years ago (2016-12-10 20:58:35 UTC) #79
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/39e086ae3eda235a490200b73b1702cbf5d2cc93 Cr-Commit-Position: refs/heads/master@{#437763}
4 years ago (2016-12-12 15:06:46 UTC) #81
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/3c957a583f692a71024bddbcf50b39a69af89d6e Cr-Commit-Position: refs/heads/master@{#437777}
4 years ago (2016-12-12 15:07:21 UTC) #83
danakj
4 years ago (2016-12-14 17:02:40 UTC) #84
Message was sent while issue was closed.
Thank you for this :)

Powered by Google App Engine
This is Rietveld 408576698