Closed Bug 1306642 Opened 8 years ago Closed 6 years ago

ASAN poisoning functions shouldn't be dllimport on Windows

Categories

(NSPR :: NSPR, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: away, Assigned: emk)

References

Details

Attachments

(4 files)

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
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. :-/
> 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.
Mmm, we can't add dllimport because declaretions in sanitizer/asan_interface.h have no dll linkages and we can't touch this file :(
Again, NSPR is in another repo and this warning spams too many directories.
Attachment #8997645 - Flags: review?(dmajor)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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+
(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
Keywords: leave-open
(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.
Keywords: leave-open
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+
Target Milestone: --- → 4.20
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-
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
The NSPR fix is iincluded in this changeset: https://hg.mozilla.org/mozilla-central/rev/c97a71755856
Attachment #9001560 - Flags: review?(dmajor)
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+
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
Depends on: 1485028
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: