Move to RLBox API to invoke functions in libGraphite so we can easily switch between unsandboxed and sandboxed versions of libGraphite. For now this task will still use the unsandboxed libGraphite included in tree. Furthermore, this bug does NOT require the returned data to be sanitized.

Comment on attachment 9084905 [details]
Bump rlbox to latest version as it is needed for Bug 1566288. r=erahm,froydnj

Revision D41660 was moved to bug 1582009. Setting attachment 9084905 [details] to obsolete.

Note that in bug 1591726, I'm proposing to eliminate the call to gr_count_unicode_characters, so that'll be one fewer API calls to handle here.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&tochange=805e56a8f30659c6a4e23df0e32419ad09076f0d&fromchange=f5d00d265686ca7ffd6186761d2b98d347455edc&selectedJob=280923420

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=280923420&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/97c087d81e8b08bc61b5e09694c32397a036bd76

task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest'
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/x86_64-w64-mingw32-clang++ -std=gnu++17 -shared -Wl,--out-implib -Wl,libxul.a -Wl,-pdb,xul.pdb -o xul.dll @/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest/xul_dll.list ./module.res -Wl,--icf=safe -lssp -fstack-protector-strong -Wl,--dynamicbase -Wl,-Xlink=-DELAYLOAD:comdlg32.dll -Wl,-Xlink=-DELAYLOAD:credui.dll -Wl,-Xlink=-DELAYLOAD:hid.dll -Wl,-Xlink=-DELAYLOAD:msimg32.dll -Wl,-Xlink=-DELAYLOAD:netapi32.dll -Wl,-Xlink=-DELAYLOAD:secur32.dll -Wl,-Xlink=-DELAYLOAD:urlmon.dll -Wl,-Xlink=-DELAYLOAD:wininet.dll -Wl,-Xlink=-DELAYLOAD:winspool.drv -Wl,-Xlink=-DELAYLOAD:oleacc.dll -Wl,-Xlink=-DELAYLOAD:api-ms-win-core-winrt-l1-1-0.dll -Wl,-Xlink=-DELAYLOAD:api-ms-win-core-winrt-string-l1-1-0.dll ../../../security/nss/lib/crmf/crmf_crmf/libcrmf.a ../../../js/src/build/libjs_static.a /builds/worker/workspace/build/src/obj-firefox/x86_64-pc-windows-gnu/debug/gkrust_gtest.lib ../../../security/libnss3.a ../../../config/external/lgpllibs/liblgpllibs.a ../../../mozglue/build/libmozglue.a -luuid -lusp10 -lgdi32 -lwinmm -lwsock32 -luserenv -lsecur32 -lavrt -lole32 -lshell32 -ldbghelp -lmpr -lhid -lrpcrt4 -lurlmon -lusp10 -lmsimg32 -lwinmm -lntdll -lcredui -lmfuuid -lwmcodecdspuuid -lstrmiids -lcrypt32 -lversion -lwinspool -lcomdlg32 -limm32 -lnetapi32 -lshlwapi -lws2_32 -ldwmapi -liphlpapi -luxtheme -lsetupapi -lsecur32 -lsensorsapi -lportabledeviceguids -lwininet -lwbemuuid -lwintrust -lwtsapi32 -llocationapi -lsapi -ldxguid -ldhcpcsvc -ld3dcompiler -loleacc -loleaut32 -ldelayimp
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - lld-link: error: duplicate symbol: _ZTWN5rlbox18rlbox_noop_sandbox11thread_dataE in ../../../gfx/thebes/Unified_cpp_gfx_thebes0.o and in ../../../gfx/thebes/Unified_cpp_gfx_thebes1.o
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - clang-9: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - /builds/worker/workspace/build/src/config/rules.mk:608: recipe for target 'xul.dll' failed
[task 2019-12-12T18:50:48.555Z] 18:50:48 ERROR - make[4]: *** [xul.dll] Error 1
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest'
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'toolkit/library/gtest/target' failed
[task 2019-12-12T18:50:48.555Z] 18:50:48 ERROR - make[3]: *** [toolkit/library/gtest/target] Error 2
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - make[3]: *** Waiting for unfinished jobs....
[task 2019-12-12T18:50:49.747Z] 18:50:49 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/js/src/gdb'
[task 2019-12-12T18:50:49.751Z] 18:50:49 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/x86_64-w64-mingw32-clang++ -std=gnu++17 -o ../../../dist/bin/gdb-tests.exe -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -ftrivial-auto-var-init=pattern -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-unknown-pragmas -Wno-unused-function -Wno-conversion-null -Wno-switch -Wno-enum-compare -Wno-gnu-zero-variadic-macro-arguments -Wno-noexcept-type -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fno-aligned-new -fms-extensions -Wno-incompatible-ms-struct -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -gcodeview -O2 -fno-omit-frame-pointer -funwind-tables gdb-tests.o Unified_cpp_js_src_gdb0.o Unified_cpp_js_src_gdb1.o -mconsole -Wl,--icf=safe -lssp -fstack-protector-strong -Wl,--dynamicbase ../build/libjs_static.a /builds/worker/workspace/build/src/obj-firefox/x86_64-pc-windows-gnu/debug/jsrust.lib -pie -Wl,-Xlink=-STACK:8388608 -Wl,-pdb,../../../dist/bin//gdb-tests.pdb ../../../mozglue/build/libmozglue.a ../../../security/libnss3.a -lm -lusp10 -lgdi32 -lwinmm -lwsock32 -lshell32 -luserenv -lws2_32
[task 2019-12-12T18:50:49.751Z] 18:50:49 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/gdb'

Also please take a look at these kind of browser chrome failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280930544&repo=autoland&lineNumber=23723

TH link: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=windows%2C10%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Ctest-windows10-64-qr%2Fdebug-mochitest-browser-chrome-e10s-2%2Cm%28bc2%29&tochange=805e56a8f30659c6a4e23df0e32419ad09076f0d&fromchange=f5d00d265686ca7ffd6186761d2b98d347455edc&selectedJob=280930805

[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - lld-link: error: duplicate symbol: _ZTWN5rlbox18rlbox_noop_sandbox11thread_dataE in ../../../gfx/thebes/Unified_cpp_gfx_thebes0.o and in ../../../gfx/thebes/Unified_cpp_gfx_thebes1.o
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - clang-9: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2019-12-12T18:50:48.555Z] 18:50:48 INFO - /builds/worker/workspace/build/src/config/rules.mk:608: recipe for target 'xul.dll' failed

OK, so our mingw32 (not regular Windows) builds are failing with duplicate symbol errors, which aren't supposed to happen. My first thought is that this is a compiler bug: somebody forgot to handle this case. But if I understand correctly, this code:

https://searchfox.org/mozilla-central/source/third_party/rlbox/include/rlbox_noop_sandbox.hpp#42

doesn't really need to use inline variables, because we can just provide the definition of thread_data out-of-line somewhere? Or do we not have appropriate places to store the definition?

It's a header only library, so it doesn't come with any source files by default.

I can add some workaround code. We could add a source file in Firefox tree where we declare this + I could add an extension point to RLBox that just declares the variable as extern.

However, I'm wondering if it's worth investigating the above compiler bug first, before trying a workaround or do you think that may be tricky/too time consuming?

(In reply to Shravan Narayan from comment #8)

It's a header only library, so it doesn't come with any source files by default.

I can add some workaround code. We could add a source file in Firefox tree where we declare this + I could add an extension point to RLBox that just declares the variable as extern.

However, I'm wondering if it's worth investigating the above compiler bug first, before trying a workaround or do you think that may be tricky/too time consuming?

We can totally investigate the compiler bug by writing a reduced test case (probably just a class with a single thread_local static inline variable?) and verifying that you get linker errors when compiling for mingw, but not for, say, Linux. Ideally you'd test this with tip clang to make sure the bug is still present upstream. If the fix is easy, we could maybe backport that, though I'm not sure of the implications for people compiling mingw Firefox themselves (e.g. Tor). Tom, do you have extra insight here?

Is the question "Can we (where we=Mozilla or Tor) apply a backported clang patch to the mingw-clang toolchain?" The answer is "yes". Mozilla will need a backport to clang 9; Tor will need a backport to clang 8.

Is the question "Is this bug caused by the mingw-clang compiler and not a bug in clang?" The answer is "possibly". If you make a reduced testcase, we have a friendly clang/mingw-clang developer who could look.

You can download the mingw-clang toolchains here:

clang-8-x64: https://tools.taskcluster.net/index/gecko.cache.level-3.toolchains.v3.linux64-clang-8-mingw-x64/latest
clang-8-x86: https://tools.taskcluster.net/index/gecko.cache.level-3.toolchains.v3.linux64-clang-8-mingw-x86/latest

clang-9 landed literally minutes ago, I think replacing the -8- above with -8- will work shortly....

The other alternative is to modify RLBox to do something like:

#ifndef RLBOX_EMBEDDER_PROVIDES_NOOP_THREAD_DATA
rlbox_noop_sandbox_thread_local& thread_data() { return thread_data };

thread_local static inline rlbox_noop_sandbox_thread_local thread_data{ 0, 0 };
#else
rlbox_noop_sandbox_thread_local& thread_data();
#endif

and provide an appropriate definition of thread_data() somewhere in Firefox using our custom thread local stuff. (The above is not exactly legal C++, but you get the idea.)

I am slightly surprised these compiled on Android, because I thought Android didn't support thread_local. Or maybe it does, but things fail at runtime?

Yup, this is similar to the extension point I had in mind.

Re Android - I believe we did actually run and pass the tests that executed this code on Android as well. But as an added precaution, I think we can do a fresh multiplatform buld and test on try before landing this again.

I tested static inline variables with the clang-8-x64 and clang-8-x86 from above links. These seem to work fine on my machine. I couldn't find the clang-9 versions through replacing the -8- in the above links though.... Any suggestions?

Also I am wondering if this is an issue that crops up due to some interaction with the unified build... the error definitely occurs when linking 2 unified_*.o files

Ok, this is confusing... I can't reproduce the issue.
@Nathan: Is try and autoland automation different somehow?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10061673a7361c9ac829b60a08528ca2bc3aedea

(In reply to Shravan Narayan from comment #15)

Ok, this is confusing... I can't reproduce the issue.
@Nathan: Is try and autoland automation different somehow?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10061673a7361c9ac829b60a08528ca2bc3aedea

Autoland was probably using clang 9, and if you haven't updated your repo to tip you are using clang 8.

I don't know why the clang 9 toolchain isn't on the taskcluster cache though... I'll look into it later today

You can get to the clang 9 toolchain from the taskcluster task page for the failing job on autoland:

https://firefox-ci-tc.services.mozilla.com/tasks/bYVOY9B7SvOMnbWo7UUaHw

But I'm puzzled as to what's going on here. I tried a simple testcase:

struct testing {
  struct rlbox_noop_sandbox_thread_local
  {
    void* sandbox;
    unsigned int last_callback_invoked;
  };

  thread_local static inline rlbox_noop_sandbox_thread_local thread_data{ 0, 0 };

  unsigned int callbacks[32];
};

testing::rlbox_noop_sandbox_thread_local f() {
  return testing::thread_data;
}

and compiled it with a mingw clang in 32-bit mode, for both clang 8 and clang 9, and the only difference between the (-m32) assembly produced was that the clang 9 version includes CFI directives for _ZTWN7testing11thread_dataE (which is "TLS wrapper function for testing::thread_data"). And there was no difference in the -m64 assembly. Maybe we're looking at a linker bug instead?

This was my suspicion as well. From the build logs, the error seems like its not in the compilation,. but actually occurs when linking 2 object files which both include the header where thread_data is defined.

Also, I am currently rebasing on bookmarks/autoland. Is this the correct way to get the latest changes or is there a better way to get to tip?

OK, so https://github.com/llvm/llvm-project/commit/341a68ca2f5b11d83350db22fb4aa1614483c1f3#diff-9fc41242b1578da3b27af3185d217205 looks like a plausible linker commit that could have broken things here, but I'm inclined to think that's not actually the problem, the problem is the declaration for the TLS wrapper function:

.section	.text$_ZTWN7testing11thread_dataE,"xr",one_only,__ZTWN7testing11thread_dataE

I'm not exactly versed in .section semantics for mingw, but I think that one_only is a mistake: we actually want something more like the semantics for template functions: take one of these things, but duplicates in the input files are OK. See for instance:

https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCSectionCOFF.cpp#L73

where one_only says we can't have duplicates, but discard is really the semantics we want (and, in fact, is how the TLS variable itself is specified:

.section	.tls$$_ZN7testing11thread_dataE,"dw",discard,__ZN7testing11thread_dataE

I'm not exactly sure how the linker could have gotten the COMDAT semantics wrong prior to the above commit -- maybe I have the wrong commit -- but I do think there's some kind of issue in LLVM itself wrt these TLS wrapper functions.

Hmm... From that description, this sounds like a bug that people haven't run into yet. While we should file the bug upstream, I'm thinking to get the current commit through, we probably have to work around this. I'll investigate alternate options such as the workaround described above

Martin, does it seem like a bug to you that TLS wrapper functions are emitted as one_only, rather than discard, as per comment 20?

@Nathan - quick question.

and provide an appropriate definition of thread_data() somewhere in Firefox using our custom thread local stuff

Looking at this now. Is there a good source location for the definition of thread_data that is not in the header? Could this go into external/config/rlbox? Any suggestions appreciated.

(In reply to Shravan Narayan from comment #23)

@Nathan - quick question.

and provide an appropriate definition of thread_data() somewhere in Firefox using our custom thread local stuff

Looking at this now. Is there a good source location for the definition of thread_data that is not in the header? Could this go into external/config/rlbox? Any suggestions appreciated.

config/external/rlbox seems like a fine place to put it; you'll have to set up config/external/moz.build to add rlbox to DIRS, and only if we're doing wasm sandboxing.

Sounds good. Thanks!

@nathan: Hmm... The workaround doesn't work either. This is now actively blocking as I'm not sure if there is another way to resolve this....

https://treeherder.mozilla.org/#/jobs?repo=try&revision=254f88f667618e40298f3255db28d00e9c07c311

Il experiment some more and see

(In reply to Shravan Narayan from comment #26)

@nathan: Hmm... The workaround doesn't work either. This is now actively blocking as I'm not sure if there is another way to resolve this....

https://treeherder.mozilla.org/#/jobs?repo=try&revision=254f88f667618e40298f3255db28d00e9c07c311

Il experiment some more and see

I haven't thoroughly looked through the patch stack, but it looks like you define the necessary RLBOX defines here:

https://hg.mozilla.org/try/rev/9d8f1f341abe546e1dc0f45cac4005d976b1dd15#l2.11

But you don't define them anywhere else to inhibit the definition from showing up when you use RLBox in libgraphite? At the very least, this revision:

https://hg.mozilla.org/try/rev/d938882b5f0d8ded08ff60bc3538d51886f7a86b

should define RLBOX_EMBEDDER_PROVIDES_TLS_STATIC_VARIABLES somewhere, I think?

That's in the child commit... From this try build it looks like class level thread locals are broken. I'll try moving this outside the class next.

Sorry, I was running out of the door this morning. I see RLBOX_EMBEDDER_PROVIDES_TLS_STATIC_VARIABLES is defined here:

https://hg.mozilla.org/try/rev/9d8f1f341abe546e1dc0f45cac4005d976b1dd15#l2.11

But there's no other definition of that macro in any of the patches in that try push that would inhibit the definition (not the declaration) from the sandbox:

https://hg.mozilla.org/try/rev/2c6b1abf6e9617d6a19b6f31b9e1e5caf7fc3903#l2.12

So you wind up with config/external/rlbox/rlbox_thread_locals.cpp defining the variable and the variable being defined in the headers and therefore in the graphite sources. Somewhere in graphite should try defining RLBOX_EMBEDDER_PROVIDES_TLS_STATIC_VARIABLES before including the RLBox headers.

@Nathan Oops, missed your message and ended up spending a few hours realizing the same thing. Some of the workarounds still cause issues with mingw, but using getter functions seems to work. I have pushed the changes and have kicked off tests which seem to be fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=185d822ee5a8be71d41792f83bf30f7f7fb69679

I think we are ready to land this again.

(In reply to Nathan Froyd [:froydnj] from comment #20)

OK, so https://github.com/llvm/llvm-project/commit/341a68ca2f5b11d83350db22fb4aa1614483c1f3#diff-9fc41242b1578da3b27af3185d217205 looks like a plausible linker commit that could have broken things here

No, that shouldn't be related to the issues here, that one only changes how sections are sorted.

.section	.text$_ZTWN7testing11thread_dataE,"xr",one_only,__ZTWN7testing11thread_dataE

I'm not exactly versed in .section semantics for mingw, but I think that one_only is a mistake: we actually want something more like the semantics for template functions: take one of these things, but duplicates in the input files are OK. See for instance:

https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCSectionCOFF.cpp#L73

where one_only says we can't have duplicates, but discard is really the semantics we want (and, in fact, is how the TLS variable itself is specified:

.section	.tls$$_ZN7testing11thread_dataE,"dw",discard,__ZN7testing11thread_dataE

I'm not exactly sure how the linker could have gotten the COMDAT semantics wrong prior to the above commit -- maybe I have the wrong commit -- but I do think there's some kind of issue in LLVM itself wrt these TLS wrapper functions.

(In reply to Nathan Froyd [:froydnj] from comment #22)

Martin, does it seem like a bug to you that TLS wrapper functions are emitted as one_only, rather than discard, as per comment 20?

Possibly - although the whole concept of inline variables and TLS wrappers is new to me.

I tried to reproduce that section type with the supplied test code, but I'm not getting any .text$_ZTWN7testing11thread_dataE whatsoever, the only COMDAT I get is .tls$$_ZN7testing11thread_dataE. I tried clang 8, 9 and nightly, with -target x86_64-w64-mingw32 -S test.cpp -o - -std=c++17 (and a few tests with i686-w64-mingw32 too). What am I missing to reproduce that section type?

(In reply to Martin Storsjö from comment #31)

(In reply to Nathan Froyd [:froydnj] from comment #22)

Martin, does it seem like a bug to you that TLS wrapper functions are emitted as one_only, rather than discard, as per comment 20?

Possibly - although the whole concept of inline variables and TLS wrappers is new to me.

I tried to reproduce that section type with the supplied test code, but I'm not getting any .text$_ZTWN7testing11thread_dataE whatsoever, the only COMDAT I get is .tls$$_ZN7testing11thread_dataE. I tried clang 8, 9 and nightly, with -target x86_64-w64-mingw32 -S test.cpp -o - -std=c++17 (and a few tests with i686-w64-mingw32 too). What am I missing to reproduce that section type?

Oh, sorry, you'll need -ffunction-sections -fdata-sections to reproduce.

(In reply to Nathan Froyd [:froydnj] from comment #32)

(In reply to Martin Storsjö from comment #31)

(In reply to Nathan Froyd [:froydnj] from comment #22)

Martin, does it seem like a bug to you that TLS wrapper functions are emitted as one_only, rather than discard, as per comment 20?

Possibly - although the whole concept of inline variables and TLS wrappers is new to me.

I tried to reproduce that section type with the supplied test code, but I'm not getting any .text$_ZTWN7testing11thread_dataE whatsoever, the only COMDAT I get is .tls$$_ZN7testing11thread_dataE. I tried clang 8, 9 and nightly, with -target x86_64-w64-mingw32 -S test.cpp -o - -std=c++17 (and a few tests with i686-w64-mingw32 too). What am I missing to reproduce that section type?

Oh, sorry, you'll need -ffunction-sections -fdata-sections to reproduce.

Ah, great, thanks! Now I can reproduce it.

If building with -S -emit-llvm, I see

define weak_odr hidden %"struct.testing::rlbox_noop_sandbox_thread_local"* @_ZTWN7testing11thread_dataE() #1 {
  ret %"struct.testing::rlbox_noop_sandbox_thread_local"* @_ZN7testing11thread_dataE
}

I haven't really seen weak_odr in use anywhere so far (and I'm not really sure how well it maps to COFF or how it is implemented), so it might be that clang should be tweaked to emit a different visibility attribute for this kind of function. Will look into that a bit later.

Yeah, I'd definitely say this is a bug. I posted an initial patch at https://reviews.llvm.org/D71572, but it turned out not to actually fix this whole case, I sent it a bit prematurely. I'll continue digging into it later.

Please note that this was backed out together with bug 1569369:
https://hg.mozilla.org/integration/autoland/rev/609b5711fb9da455d234a0ceb99c804417c9b95c

On the backed out push, there were also Tier2 webgl assertion failures
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281726044&repo=autoland&lineNumber=1092

Thanks for the info. I think this needs further investigation from my side, as I haven't run into these errors on try. Will follow up.

https://hg.mozilla.org/mozilla-central/rev/c11db5fee856

https://reviews.llvm.org/D71572 was landed in Clang yesterday, which hopefully should fix the root cause to this issue - even though you've already worked around it.