Closed Bug 1485028 Opened Last year Closed Last year

Fix warnings that are specific to clang-cl ASAN builds

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attachment #9002775 - Flags: review?(core-build-config-reviews)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Summary: Fix warnings that are specific clang-cl ASAN builds → Fix warnings that are specific to clang-cl ASAN builds
I assume that the change in XPCJSContext.cpp is to silence this warning:

22:21:06     INFO -  z:/build/build/src/js/xpconnect/src/XPCJSContext.cpp(949,1):  warning: unused function 'GetWindowsStackSize' [-Wunused-function]

But it seems like a legitimate warning. The place where GetWindowsStackSize would be called is behind an ifndef MOZ_ASAN, but I think this is historical accident from when ASan was only available on Linux builds: https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/xpconnect/src/XPCJSContext.cpp#1098

It would be nice if we could make Windows ASan builds use GetWindowsStackSize just like the non-ASan builds. Do the tests pass if you re-arrange the checks so that defined(XP_WIN) comes before defined(MOZ_ASAN)?
Attachment #9002823 - Flags: review?(core-build-config-reviews)
Attachment #9002775 - Attachment is obsolete: true
Attachment #9002775 - Flags: review?(core-build-config-reviews)
Attachment #9002823 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 9002822 [details] [diff] [review]
Use GetWindowsStackSize() on Windows even with ASAN builds

Review of attachment 9002822 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSContext.cpp
@@ +1091,5 @@
> +    // 1MB is the default stack size on Windows. We use the -STACK linker flag
> +    // (see WIN32_EXE_LDFLAGS in config/config.mk) to request a larger stack,
> +    // so we determine the stack size at runtime.
> +    const size_t kStackQuota = GetWindowsStackSize();
> +    const size_t kTrustedScriptBuffer = (sizeof(size_t) == 8) ? 180 * 1024   //win64

I wonder if it makes sense to have kTrustedScriptBuffer be 450 * 1024 in the ASan case, like we do in the Linux code right above this. If this is green it's probably fine to leave it as is, though.
Attachment #9002822 - Flags: review?(jdemooij) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42366ac19e9
Use GetWindowsStackSize() on Windows even with ASAN builds. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec51e192b5d
Fix warnings that are specific to clang-cl ASAN builds. r=dmajor
https://hg.mozilla.org/mozilla-central/rev/e42366ac19e9
https://hg.mozilla.org/mozilla-central/rev/eec51e192b5d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.