1306642 - ASAN poisoning functions shouldn't be dllimport on Windows

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[(Away)]

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

[(not currently active) Ted Mielczarek]

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. :-/

Comment 2

[Masatoshi Kimura [:emk]]

> 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.

Comment 3

[(Away)]

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.

Comment 4

[Masatoshi Kimura [:emk]]

Mmm, we can't add dllimport because declaretions in sanitizer/asan_interface.h have no dll linkages and we can't touch this file :(

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Temporarily allow warnings from MemoryChecking.h

Again, NSPR is in another repo and this warning spams too many directories.

Comment 7

[(Away)]

Comment on attachment 8997645 [details] [diff] [review]
Temporarily allow warnings from MemoryChecking.h

Please surround this area with a `#pragma clang diagnostic {push,pop}`.

Comment 8

[Masatoshi Kimura [:emk]]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd8445b019f
Temporarily allow warnings from MemoryChecking.h. r=dmajor

Comment 10

[(Away)]

(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

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5bd8445b019f

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Stop ASAN poisoning functions from using dllimport on Windows

Comment 13

[Kai Engert (:KaiE:)]

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.

Comment 14

[Kai Engert (:KaiE:)]

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.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: 1306642-v2.patch

This fixed version of your patch builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b0192c92acf1e49c463178c1f7387a75f70bbf9

Comment 16

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/6e31156d7002

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Revert changeset 5bd8445b019f as final NSPR fix has landed

The NSPR fix is iincluded in this changeset:
https://hg.mozilla.org/mozilla-central/rev/c97a71755856

Comment 18

[(Away)]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/635d66906fe4
Backed out changeset 5bd8445b019f as final NSPR fix has landed. r=dmajor

Comment 20

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/635d66906fe4