Reassess MSVC_DISABLE_OPTIMIZE |
|||
Issue descriptionIn the VC++ world MSVC_DISABLE_OPTIMIZE was often used to stop a function from being inlined. However this doesn't work with clang-cl. We could try altering clang-cl to make it respect the VC++ semantics but I think it makes more sense to use the more explicit NOINLINE tag if that is what we want. A search for MSVC_DISABLE_OPTIMIZE finds a moderate number of uses. This was noticed while investigating bug 873286 - CrashOutOfMemory and others were being inlined, leading to confusing call stacks.
,
Aug 15
taking per Bruce's suggestion.
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a86573a2569ead56ef2b3273f6613f4ffb4a3e38 commit a86573a2569ead56ef2b3273f6613f4ffb4a3e38 Author: David Bienvenu <davidbienvenu@chromium.org> Date: Mon Aug 20 19:34:00 2018 Update no-inlining for a clang-cl world With VC++ marking a function as non-optimized with MSVC_DISABLE_OPTIMIZE would prevent it from being inlined, however with clang-cl that does not work, leading to confusing crash stacks. The fix is to use NOINLINE instead of MSVC_DISABLE_OPTIMIZE, which is more explicit and specific anyway. Bug: 873359 Change-Id: I374c15af002c1864abac1d9a26f20026cc941a07 Reviewed-on: https://chromium-review.googlesource.com/1179961 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org> Cr-Commit-Position: refs/heads/master@{#584531} [modify] https://crrev.com/a86573a2569ead56ef2b3273f6613f4ffb4a3e38/chrome/browser/metrics/thread_watcher_report_hang.cc [modify] https://crrev.com/a86573a2569ead56ef2b3273f6613f4ffb4a3e38/chrome/common/logging_chrome.cc
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1bd0f306317741d2a300892478cc8f1ace2c409d commit 1bd0f306317741d2a300892478cc8f1ace2c409d Author: David Bienvenu <davidbienvenu@chromium.org> Date: Tue Aug 21 02:46:26 2018 Update no-inlining for a clang-cl world With VC++ marking a function as non-optimized with MSVC_DISABLE_OPTIMIZE would prevent it from being inlined, however with clang-cl that does not work, leading to confusing crash stacks. The fix is to remove MSVC_DISABLE_OPTIMIZE; these functions were already marked as NOINLINE. Bug: 873359 Change-Id: I00995a17463572545933956cdec0e7e13825abe7 Reviewed-on: https://chromium-review.googlesource.com/1182302 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org> Cr-Commit-Position: refs/heads/master@{#584607} [modify] https://crrev.com/1bd0f306317741d2a300892478cc8f1ace2c409d/content/browser/browser_main_loop.cc [modify] https://crrev.com/1bd0f306317741d2a300892478cc8f1ace2c409d/content/browser/browser_process_sub_thread.cc
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af3c256b81635d6d9457a6e6c838871ab6002dc9 commit af3c256b81635d6d9457a6e6c838871ab6002dc9 Author: David Bienvenu <davidbienvenu@chromium.org> Date: Fri Aug 24 14:20:39 2018 Update no-inlining for a clang-cl world With VC++ marking a function as non-optimized with MSVC_DISABLE_OPTIMIZE would prevent it from being inlined, however with clang-cl that does not work, leading to confusing crash stacks. The fix is to use NOINLINE instead of MSVC_DISABLE_OPTIMIZE, which is more explicit and specific anyway. Bug: 873359 Change-Id: Ieb5c29e3212596f928a636d15143d717bd2c744c Reviewed-on: https://chromium-review.googlesource.com/1187265 Reviewed-by: Alexander Timin <altimin@chromium.org> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org> Cr-Commit-Position: refs/heads/master@{#585831} [modify] https://crrev.com/af3c256b81635d6d9457a6e6c838871ab6002dc9/base/task/sequence_manager/sequence_manager_impl.cc
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d207c9205366b3025c677560e6631be4e046b83 commit 6d207c9205366b3025c677560e6631be4e046b83 Author: David Bienvenu <davidbienvenu@chromium.org> Date: Wed Sep 12 16:11:15 2018 Update no-inlining for a clang-cl world With VC++ marking a function as non-optimized with MSVC_DISABLE_OPTIMIZE would prevent it from being inlined, however with clang-cl that does not work, leading to confusing crash stacks. The fix is to use NOINLINE instead of MSVC_DISABLE_OPTIMIZE, which is more explicit and specific anyway. Bug: 873359 Change-Id: Ibed2c9e4cb30254b53cfecc44439757cb7504fb6 Reviewed-on: https://chromium-review.googlesource.com/1188437 Reviewed-by: Carlos Pizano <cpu@chromium.org> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org> Cr-Commit-Position: refs/heads/master@{#590709} [modify] https://crrev.com/6d207c9205366b3025c677560e6631be4e046b83/components/crash/content/app/crashpad_win.cc
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fcd272e917639cb9fe5df9b512a5568e9c45806b commit fcd272e917639cb9fe5df9b512a5568e9c45806b Author: David Bienvenu <davidbienvenu@chromium.org> Date: Tue Sep 18 16:22:42 2018 Update no-inlining for a clang-cl world With VC++ marking a function as non-optimized with MSVC_DISABLE_OPTIMIZE would prevent it from being inlined, however with clang-cl that does not work, leading to confusing crash stacks. The fix is to use NOINLINE instead of MSVC_DISABLE_OPTIMIZE, which is more explicit and specific anyway. Bug: 873359 Change-Id: Idaaf11fb0807990fb72ec3b12515b46a5814356c Reviewed-on: https://chromium-review.googlesource.com/1228973 Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org> Commit-Queue: David Bienvenu <davidbienvenu@chromium.org> Cr-Commit-Position: refs/heads/master@{#592066} [modify] https://crrev.com/fcd272e917639cb9fe5df9b512a5568e9c45806b/chrome/chrome_watcher/chrome_watcher_main.cc
,
Sep 18
I've removed all uses of MSVC_{ENABLE|DISABLE}_OPTIMIZE - @brucedawson, the remaining question is should I remove the #defines in base/compiler_specific.h?
It's also defined in src/third_party/pdfium/third_party/base/compiler_specific.h - I'm guessing I should leave that alone.
,
Sep 18
The macros can be useful in certain debugging situations, although a user could (if they know how) use #pragma optimize to the same effect. I don't have strong feelings either way. You could also put in a comment saying that the macros are usually the wrong solution, but I'd probably just leave well enough alone. For something like this developers will copy other examples or will use what works, so now that the bad examples are cleaned up we're probably fine.
,
Sep 19
thx, sgtm. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Aug 14