ThinLTO seems to break computed goto

[glandium]

Bugzilla Link	39241
Resolution	FIXED
Resolved on	Oct 26, 2018 09:52
Version	unspecified
OS	Linux
Blocks	#38454
CC	@eleviant,@pawelsopensource,@rnk,@sylvestre,@tstellar
Fixed by commit(s)	r344325 r345401

Extended Description

STR:
git clone https://github.com/silnrsi/graphite
cd graphite
mkdir build
cd build
cmake -GNinja -DCMAKE_C_COMPILER=/path/to/bin/clang -DCMAKE_CXX_COMPILER=/path/to/bin/clang++ -DCMAKE_CXX_FLAGS=-flto=thin -DCMAKE_SHARED_LINKER_FLAGS=-flto=thin -DCMAKE_EXE_LINKER_FLAGS=-flto=thin -DCMAKE_MODULE_LINKER_FLAGS=-flto=thin -DCMAKE_C_FLAGS=-flto=thin -DCMAKE_AR=/path/to/bin/llvm-ar -DCMAKE_RANLIB=/path/to/bin/llvm-ranlib ..
ninja
ninja test

This results in many tests failing (84 out of 93 on my machine). A non-LTO build only fails only 8 tests on my machine.

This worked fine with clang 6, so I bisected it down to:

commit 11b0e47 (refs/bisect/bad)
Author: Eugene Leviant [email protected]
Date: Fri Feb 16 08:11:04 2018 +0000

[ThinLTO] Import global variables

Differential revision: https://reviews.llvm.org/D43077


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@325320 91177308-0d34-0410-b5e6-96231b3b80d8

I haven't dug into how exactly the generated code is broken, but adding the following to the cmake command line makes it work: -DGRAPHITE2_VM_TYPE=call
That makes call_machine.cpp be used instead of direct_machine.cpp. The main difference between both is that the latter is using computed gotos.

[glandium]

assigned to @eleviant

[glandium]

A workaround is to add attribute((noinline)) to direct_run in direct_machine.cpp.

[glandium]

A workaround is to add attribute((noinline)) to direct_run in
direct_machine.cpp.

... which, in fact, seems to be the core problem: here is what the GCC docs say about labels as values:

The &&foo expressions for the same label might have different values if the containing function is inlined or cloned. If a program relies on them being always the same, attribute((noinline,noclone)) should be used to prevent inlining and cloning.

https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html

That said, it also follows with:
If &&foo is used in a static variable initializer, inlining and cloning is forbidden.

And here it is, so arguably, clang should forbid inlining here.

[glandium]

I'm not sure if this is limited to is_return or if other uses of Machine::getOpcodeTable have the same problem, but specifically for is_return, if I add attribute((noinline)) to it, this is what it compiles to:

Dump of assembler code for function (anonymous namespace)::is_return(void*):
=> 0x00007f0e7414a1c0 <+0>: cmp $0x1,%rdi
0x00007f0e7414a1c4 <+4>: sete %al
0x00007f0e7414a1c7 <+7>: retq

This can't be right.

[glandium]

STR:
git clone https://github.com/silnrsi/graphite

As I've submitted a PR to add attribute((noinline)), the STR may eventually fail to reproduce. Checkout any commit up to (and including) 201162775ae48c1ac364c4af500b85f8224756c4 in order to reproduce.

[eleviant]

I'm looking at it at the moment. Will get back when I find something.

[eleviant]

Possible fix: https://reviews.llvm.org/D53139

[glandium]

Interestingly, the patch works for the STR I gave, but it doesn't when building Firefox with ThinLTO enabled.

[eleviant]

Are you using thinlto cache? If so please clear it first and rebuild from scratch. If nothing helps, please create an issue.

[glandium]

Not using a ThinLTO cache. But FWIW, I filed this bug because we originally had this problem in Firefox (in the bundled graphite), which I reproduced with graphite separately.

[eleviant]

Ok, please file a bug then

[glandium]

But this bug is still open, and this is the same problem, on the same function, inlined the same wrong way in the same source file, but linked in something bigger (and with the same workaround with attribute((noinline))).

[eleviant]

Do you have steps to reproduce this with Firefox?

[glandium]

$ hg clone https://hg.mozilla.org/mozilla-central
$ cd mozilla-central
$ ./mach bootstrap
$ AR=llvm-ar NM=llvm-nm RANLIB=llvm-ranlib ./mach configure --enable-lto --enable-release
$ ./mach build
$ ./mach test layout/reftests/text/graphite-01.html

(--enable-lto enables thinlto, not full lto)

A failing build will say something like "FAIL image comparison, max difference: 255, number of differing pixels: 896"

[glandium]

Actually, I can't reproduce locally, there might be some other cache involved in the failure I got on Firefox. Waiting for confirmation.

[glandium]

Ok, sorry for the noise, there was a separate level of caching that was breaking my test of the patched version of clang. I now confirmed the patch fixed it for Firefox too.

[tstellar]

Teresa, is this OK to merge to the release_70 branch?

https://reviews.llvm.org/rL344325

[llvmbot]

Yes thanks

[tstellar]

Merged: r345401