Closed
Bug 1306642
Opened 8 years ago
Closed 6 years ago
ASAN poisoning functions shouldn't be dllimport on Windows
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.20
People
(Reporter: away, Assigned: emk)
References
Details
Attachments
(4 files)
(deleted),
patch
|
away
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
KaiE
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
away
:
review+
|
Details | Diff | Splinter Review |
19:25.41 In file included from d:/build/msys/s/central/layout/base/nsPresArena.h:23:
19:25.41 d:/build/msys/s/central/obj/asan/dist/include/nspr\plarena.h(103,17): warning(clang): redeclaration of '__asan_poison_memory_region' should not add 'dllimport' attribute [-Wdll-attribute-on-redeclaration]
19:25.41 PR_IMPORT(void) __asan_poison_memory_region(void const volatile *addr, size_t size);
19:25.41 ^
19:25.41 d:/build/msys/s/central/obj/asan/dist/include\mozilla/MemoryChecking.h(53,1): note(clang): previous declaration is here
19:25.41 __asan_poison_memory_region(void const volatile *addr, size_t size);
19:25.41 ^
See https://hg.mozilla.org/mozilla-central/rev/c32fd9b9c355
Comment 1•8 years ago
|
||
We get similar warnings from everything in NSPR when we link it into a single sharedlib with NSS, since they're all declspec(dllimport). I tried to fix that in bug 1237863 but the fix wasn't correct so it caused bug 1242802. :-/
Assignee | ||
Comment 2•6 years ago
|
||
> ASAN poisoning functions shouldn't be dllimport on Windows
Why not? We are importing ASAN poisoning functions from a DLL (clang_rt.asan_dynamic-*.dll). So we should mark them dllimport.
https://hg.mozilla.org/mozilla-central/rev/c32fd9b9c355#l1.13
>+// In clang-cl based ASAN, we link against the memory poisoning functions
>+// statically.
This comment is plain wrong.
In the bug title I was probably just repeating what the compiler warning said. I think you're right that in practice these really are imports from a DLL.
Assignee | ||
Comment 4•6 years ago
|
||
Mmm, we can't add dllimport because declaretions in sanitizer/asan_interface.h have no dll linkages and we can't touch this file :(
Assignee | ||
Comment 5•6 years ago
|
||
Again, NSPR is in another repo and this warning spams too many directories.
Attachment #8997645 -
Flags: review?(dmajor)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Blocks: winclang-werror
Comment on attachment 8997645 [details] [diff] [review]
Temporarily allow warnings from MemoryChecking.h
Please surround this area with a `#pragma clang diagnostic {push,pop}`.
Attachment #8997645 -
Flags: review?(dmajor) → review+
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to David Major [:dmajor] from comment #7)
> Comment on attachment 8997645 [details] [diff] [review]
> Temporarily allow warnings from MemoryChecking.h
>
> Please surround this area with a `#pragma clang diagnostic {push,pop}`.
I had to remove `#pragma clang diagnostic {push,pop}` because plarena.h may be included after MemoryChecking.h.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd8445b019f
Temporarily allow warnings from MemoryChecking.h. r=dmajor
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Reporter | ||
Comment 10•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #8)
> I had to remove `#pragma clang diagnostic {push,pop}` because plarena.h may
> be included after MemoryChecking.h.
Oh, good point... that's pretty unpleasant though. Hopefully this change doesn't live for too long.
Comment 11•6 years ago
|
||
bugherder |
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8998473 -
Flags: review?(kaie)
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 13•6 years ago
|
||
Comment on attachment 8998473 [details] [diff] [review]
Stop ASAN poisoning functions from using dllimport on Windows
IIUC this change achieves consistency with another header file, that is included earlier, and which doesn't use dllimport. This is reasonable. r=kaie
I'll handle uplifting. I'll run a try build first.
Attachment #8998473 -
Flags: review?(kaie) → review+
Updated•6 years ago
|
Target Milestone: --- → 4.20
Comment 14•6 years ago
|
||
Comment on attachment 8998473 [details] [diff] [review]
Stop ASAN poisoning functions from using dllimport on Windows
The test produced build failures. Instead of empty, maybe you need to define it as "type_".
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25298b5d7b870fd0eb6b72f920d0f8df4905e5b5&selectedJob=194109937
16:08:49 INFO - z:/build/build/src/sccache2/sccache.exe z:/build/build/src/clang/bin/clang-cl.exe -fms-compatibility-version=19.13.26128 -FoUnified_cpp_certverifier0.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 '-DDLL_PREFIX=""' '-DDLL_SUFFIX=".dll"' -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/security/certverifier -Iz:/build/build/src/obj-firefox/security/certverifier -Iz:/build/build/src/security/manager/ssl -Iz:/build/build/src/security/pkix/include -Iz:/build/build/src/security/pkix/lib -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -U_FORTIFY_SOURCE -Xclang -fno-common -Qunused-arguments -fsanitize=address -fsanitize-blacklist=z:/build/build/src/build/sanitizers/asan_blacklist_win.txt -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -U_FORTIFY_SOURCE -Xclang -fno-common -W3 -Gy -Zc:inline -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -wd4065 -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O2 -gline-tables-only -Oy- -W4 -wd4324 -wd4355 -wd4464 -wd4480 -wd4481 -wd4510 -wd4512 -wd4514 -wd4610 -wd4619 -wd4623 -wd4625 -wd4626 -wd4628 -wd4640 -wd4710 -wd4711 -wd4820 -wd4548 -wd4668 -wd4987 -wd4456 -wd4458 -wd4061 -wd4365 -wd4774 -wd4100 -wd4127 -wd4946 -Xclang -MP -Xclang -dependency-file -Xclang .deps/Unified_cpp_certverifier0.obj.pp -Xclang -MT -Xclang Unified_cpp_certverifier0.obj z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49 INFO - In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:16:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:13:
16:08:49 INFO - z:/build/build/src/obj-firefox/dist/include/nspr\plarena.h(112,26): error: C++ requires a type specifier for all declarations
16:08:49 INFO - PL_ASAN_VISIBILITY(void) __asan_poison_memory_region(
16:08:49 INFO - ^
16:08:49 INFO - z:/build/build/src/obj-firefox/dist/include/nspr\plarena.h(114,26): error: C++ requires a type specifier for all declarations
16:08:49 INFO - PL_ASAN_VISIBILITY(void) __asan_unpoison_memory_region(
16:08:49 INFO - ^
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49 INFO - In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:16:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:21:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss/keyt.h:8:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss/keythi.h:9:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss/pkcs11t.h:1797:
16:08:49 INFO - z:/build/build/src/obj-firefox/dist/include/nss/pkcs11n.h(440,9): warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49 INFO - #pragma deprecated(CKT_NSS_UNTRUSTED, CKT_NSS_MUST_VERIFY, CKT_NSS_VALID)
16:08:49 INFO - ^
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49 INFO - In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:18:
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/dist/include/nss\cryptohi.h:11:
16:08:49 INFO - z:/build/build/src/obj-firefox/dist/include/nss/blapit.h(66,9): warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49 INFO - #pragma deprecated(DSA_SUBPRIME_LEN, DSA_SIGNATURE_LEN, DSA_QBITS)
16:08:49 INFO - ^
16:08:49 INFO - In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
16:08:49 INFO - In file included from z:/build/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:16:
16:08:49 INFO - z:/build/build/src/obj-firefox/dist/include/nss\certdb.h(34,9): warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49 INFO - #pragma deprecated(CERTDB_VALID_PEER)
16:08:49 INFO - ^
16:08:49 INFO - 3 warnings and 2 errors generated.
Attachment #8998473 -
Flags: review+ → review-
Comment 15•6 years ago
|
||
This fixed version of your patch builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b0192c92acf1e49c463178c1f7387a75f70bbf9
Attachment #9001364 -
Flags: review+
Comment 16•6 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•6 years ago
|
||
The NSPR fix is iincluded in this changeset:
https://hg.mozilla.org/mozilla-central/rev/c97a71755856
Attachment #9001560 -
Flags: review?(dmajor)
Reporter | ||
Comment 18•6 years ago
|
||
Comment on attachment 9001560 [details] [diff] [review]
Revert changeset 5bd8445b019f as final NSPR fix has landed
Thanks!
If you s/Revert/Back out/ in the commit message then our hgweb can link the original changeset to this one.
Attachment #9001560 -
Flags: review?(dmajor) → review+
Comment 19•6 years ago
|
||
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/635d66906fe4
Backed out changeset 5bd8445b019f as final NSPR fix has landed. r=dmajor
Comment 20•6 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•