MSVC produces different output with CHECK vs RELEASE_ASSERT |
|||||
Issue descriptionhttps://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TL0NkNXIT1w/mbMCKqymBQAJ has the details If you have a destructor then some extra code gets generated sometimes. Is dumb.
,
Dec 9 2016
,
Dec 9 2016
,
Dec 9 2016
Patch: https://codereview.chromium.org/2561963002 Generated by the following regexes + some manual ones they missed and some manual corrections. # misses multi-line checks git grep -E ' P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\(.*?\))($| <<)' | cut -f1 -d:|sort|uniq> files # run these each repeatedly until no new results. for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^"]*)";!\1// \5\n\2;!' $i; done for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)";!\1// \5\6\n\2;!' $i; done for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)";!\1// \5\6\7\n\2;!' $i; done for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)"[ \n]*(?:<<[ \n]*)?"([^"]*)";!\1// \5\6\7\8\n\2;!' $i; done # CHECK() << "this is " << caught_here(); for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^":]*)"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5|\6|.\n\2;!' $i; done # CHECK() << "this is " "also" << caught_here(); for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^":]*)"[ \n]*(?:<<[ \n]*)?"([^":]*)"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5\6|\7|.\n\2;!' $i; done # CHECK() << "Complicated thing: " << caught_here(); should become // Complicated thing. for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([A-Z][^"]*): ?"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5.\n\2;!' $i; done # CHECK() << "complicated thing: " << caught_here(); should become // complicated thing. for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^"]+ [^"]+): ?"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5.\n\2;!' $i; done # CHECK() << "or " "this: " << caught_here(); should become // or this. for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^":]*)"[ \n]*(?:<<[ \n]*)?"([^"]*): ?"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5\6.\n\2;!' $i; done # CHECK() << "this is " << also << " caught " << here(); for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^":]*)"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+)[ \n]*<<[ \n]*"([^":]*)"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+);!\1// \5|\6|\7|\8|.\n\2;!' $i; done # CHECK() << "this is " << also << " caught now."; for i in `cat files`; do perl -0777 -i.original -pe 's!(\n +)P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\((?:<?[^<;])+?)[ \n]*<<[ \n]*"([^":]*)"[ \n]*<<[ \n]*([a-z0-9A-Z_.\[\]()>-]+)[ \n]*<<[ \n]*"([^":]*)";!\1// \5|\6|\7\n\2;!' $i; done # drop the rest. again, run it multiple times to get them all in each file... for i in `cat files`; do perl -0777 -i.original -pe 's/ P?(CHECK(_(EQ|NE|GT|GE|LE|LT))?\(.*?\))[ \n]*<<(.*?|\n)*;/ \1;/' $i; done
,
Dec 9 2016
Would it be hard to shard this change? I looked through the mega CL [1], and some of the drive-by feedback is already revealing things that should either be DCHECKs or properly handled (IMO). Maybe if we shard this work, directory OWNERS can also clean up the things that really shouldn't be CHECKs? [1] https://codereview.chromium.org/2561963002/
,
Dec 9 2016
Possible proposal here: https://codereview.chromium.org/2559323007/ (I might well be missing some subtlety here though, the macros in logging.h make my head hurt).
,
Dec 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39e086ae3eda235a490200b73b1702cbf5d2cc93 commit 39e086ae3eda235a490200b73b1702cbf5d2cc93 Author: scottmg <scottmg@chromium.org> Date: Sat Dec 10 08:29:25 2016 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 Cr-Commit-Position: refs/heads/master@{#437763} [modify] https://crrev.com/39e086ae3eda235a490200b73b1702cbf5d2cc93/base/check_example.cc [modify] https://crrev.com/39e086ae3eda235a490200b73b1702cbf5d2cc93/base/logging.h
,
Dec 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/314c528dc177ac458d2449a8c4d937378d3b8c5e commit 314c528dc177ac458d2449a8c4d937378d3b8c5e Author: primiano <primiano@chromium.org> Date: Sat Dec 10 10:30:17 2016 Revert of Improve EAT_STREAM_PARAMETERS for Windows x86 (patchset #10 id:240001 of https://codereview.chromium.org/2559323007/ ) Reason for revert: Broke D*LOG on a bunch of bots building non-official builds. See https://codereview.chromium.org/2559323007/#msg60 and https://codereview.chromium.org/2559323007/#msg61 for more context. Original issue's 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 TBR=dcheng@chromium.org,scottmg@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 672699 Review-Url: https://codereview.chromium.org/2569583002 Cr-Commit-Position: refs/heads/master@{#437764} [modify] https://crrev.com/314c528dc177ac458d2449a8c4d937378d3b8c5e/base/check_example.cc [modify] https://crrev.com/314c528dc177ac458d2449a8c4d937378d3b8c5e/base/logging.h
,
Dec 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3c957a583f692a71024bddbcf50b39a69af89d6e commit 3c957a583f692a71024bddbcf50b39a69af89d6e Author: scottmg <scottmg@chromium.org> Date: Sat Dec 10 20:57:59 2016 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 Review-Url: https://codereview.chromium.org/2559323007 Cr-Commit-Position: refs/heads/master@{#437777} [modify] https://crrev.com/3c957a583f692a71024bddbcf50b39a69af89d6e/base/check_example.cc [modify] https://crrev.com/3c957a583f692a71024bddbcf50b39a69af89d6e/base/logging.cc [modify] https://crrev.com/3c957a583f692a71024bddbcf50b39a69af89d6e/base/logging.h
,
Dec 11 2016
After https://chromium.googlesource.com/chromium/src.git/+/3c957a583f692a71024bddbcf50b39a69af89d6e above, I compared generated code in check_example.exe and in chrome_child.dll (using https://codereview.chromium.org/2565883002/) and it both cases a function using CHECK and one using RELEASE_ASSERT get collapsed into the same implementation by COMDAT folding. I checked on is_official_build=true target_cpu="x86". target_cpu="x64" was already OK before we made this change. So, at least for Windows, it should be OK to convert RELEASE_ASSERT to CHECK now.
,
Dec 19 2016
Windows was the only platform that saw any difference so we should be good.
,
Dec 19 2016
,
Dec 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb7c529ef85e7df2b109e549cc94d90f4a1e6e24 commit cb7c529ef85e7df2b109e549cc94d90f4a1e6e24 Author: danakj <danakj@chromium.org> Date: Tue Dec 20 19:05:35 2016 Wrap the conditional in CHECK with UNLIKELY since it is. This matches what was done in RELEASE_ASSERT in blink. No perf bots noticed this being missing (it does nothing on windows which is where perf bots noticed anything) but it won't hurt right. R=dcheng@chromium.org BUG= 672699 Review-Url: https://codereview.chromium.org/2589943002 Cr-Commit-Position: refs/heads/master@{#439860} [modify] https://crrev.com/cb7c529ef85e7df2b109e549cc94d90f4a1e6e24/base/logging.h |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by danakj@chromium.org
, Dec 9 2016