What is the behavior when building swiftshader without D29240? And what is the behavior when linking with gold?

D29240 has no effect. The issue is reproduced with and without it applied; with or without symbols.

The behavior between lld and gold is equivalent.

My standalone reproducer:

$ cat main.cpp
class Display {
 public:
  static void Foo();

  virtual void Bar();

 private:
  Display() {}
};

void Display::Foo() {
  static Display display;
}

void Display::Bar() { }

__attribute__((destructor)) void detachProcess() {
  Display::Foo();
}

bool ThisSymbolIsUndefined();

class UnusedClass {
 public:
  virtual void UnusedMethod();
};

void UnusedClass::UnusedMethod() {
  ThisSymbolIsUndefined();
}

Building it with ThinLTO:
$ cat build_thin.sh
#!/bin/bash

set -ue

echo "Building with ThinLTO..."

mkdir -p thinlto
cd thinlto

clang++ -fPIC -flto=thin -pthread -O2 -fno-ident -fdata-sections -ffunction-sections -fvisibility=hidden -Werror -Wall -Wno-unused-variable -std=c++11 -fvisibility-inlines-hidden -c ../main.cpp -o main.o

clang++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--no-as-needed -lpthread -Wl,--as-needed -fuse-ld=lld -Wl,--icf=all -flto=thin -Wl,-mllvm,-function-sections -m64 -pthread -Wl,-O1 -Wl,--gc-sections -Werror -o "./libswiftshader_libEGL.so" -Wl,-soname="libswiftshader_libEGL.so" main.o && echo "OK" || ( echo "Failed" && exit 1 )

Building it with FullLTO:

$ cat build_full.sh
#!/bin/bash

set -ue

echo "Building with Full LTO and LLD..."

mkdir -p full
cd full

clang++ -fPIC -flto -pthread -O2 -fno-ident -fdata-sections -ffunction-sections -fvisibility=hidden -Werror -Wall -Wno-unused-variable -std=c++11 -fvisibility-inlines-hidden -c ../main.cpp -o main.o

clang++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--no-as-needed -lpthread -Wl,--as-needed -fuse-ld=lld -Wl,--icf=all -flto -Wl,-mllvm,-function-sections -m64 -pthread -Wl,-O1 -Wl,--gc-sections -Werror -o "./libswiftshader_libEGL.so" -Wl,-soname="libswiftshader_libEGL.so" main.o && echo "OK" || ( echo "Failed" && exit 1 )

These files are attached too.

Deleted: main.cpp 406 bytes

main.cpp 406 bytes View Download

Deleted: build_thin.sh 670 bytes

build_thin.sh 670 bytes View Download

Deleted: build_full.sh 663 bytes

build_full.sh 663 bytes View Download

The output from the scripts I see:

$ ./build_full.sh ; ./build_thin.sh ; ./build_thin_gold.sh
Building with Full LTO and LLD...
OK
Building with ThinLTO...
/usr/local/google/home/krasin/src/llvm.org/release-build/bin/ld.lld: error: main.o0:(function UnusedClass::UnusedMethod()): undefined symbol 'ThisSymbolIsUndefined()'
clang-5.0: error: linker command failed with exit code 1 (use -v to see invocation)
Failed
Building with ThinLTO and Gold...
/tmp/lto-llvm-9fa1f6.o:main.o:function UnusedClass::UnusedMethod(): error: undefined reference to 'ThisSymbolIsUndefined()'
clang-5.0: error: linker command failed with exit code 1 (use -v to see invocation)
Failed

Deleted: build_thin_gold.sh 685 bytes

build_thin_gold.sh 685 bytes View Download

creduce gives a somewhat smaller version:

$ cat main.cpp
class b {
public:
  virtual void c();
  b() {}
};
void b::c() {}
__attribute__((destructor)) void d() { static b a; }
void fn2();
class e {
  virtual void f();
};
void e::f() { fn2(); }

It's essentially the same, but it was able to get rid of a private constructor.

Deleted: main.cpp 186 bytes

main.cpp 186 bytes View Download

Setting the visibility instead of declaring d() a destructor works too:

class b {
public:
  virtual void c();
  b() {}
};
void b::c() {}
 __attribute__ ((visibility("default"))) void d() { static b a; }
void fn2();
class e {
  virtual void f();
};
void e::f() { fn2(); }

For the reproducer from #5, I have observed the temp files from lld and one piece stands out for the FullLTO:

define hidden void @_ZN1e1fEv(%class.b* nocapture readnone %this) unnamed_addr #1 align 2 {
entry:
  tail call void @_Z3fn2v()
  ret void
}

The function f belong to class e which can be infered by looking at the source or at the function signature:

$ echo _ZN1e1fEv | c++filt
e::f()

FullLTO somehow messes it up and passes %class.b* nocapture readnone %this into e::f(). At the same time, ThinLTO is doing it correct:
define hidden void @_ZN1e1fEv(%class.e* nocapture readnone %this) unnamed_addr #1 align 2 {
entry:
  tail call void @_Z3fn2v()
  ret void
}

That is mostly harmless as the vtables are still different and already being used in the call sites, but it confusing at best and potentially materially wrong.

This is due to the new split module generated for vtable type metadata as of r292662. The source module is split into a ThinLTO module and a Regular LTO module. The ThinLTO module contains the definition of @_ZN1e1fEv, with the reference to undefined fn2. The Regular LTO module contains a reference to @_ZN1e1fEv, from the vtable of e. Therefore, _ZN1e1fEv and class e can no longer be removed.

Compiling with "-Xclang -fno-lto-unit" lets this test pass.

Thank you for looking into this, Teresa! You're right, adding this flag makes the reproducer to link successfully.

I believe that Peter has some ideas how to fix this properly (like, moving the vtable definitions into the ThinLTO part). We should probably discuss this.

Summary of our chat on how to address this. Basically we want to include the defs in the regular LTO module in the summary as well, so that index-based dead symbol analysis can see all the uses and defs in both modules in the bitcode file, and teach regular LTO how to remove the dead symbols. I think this involves the following steps:
1) Add a flag to the summary to note whether it is defined in the regular or thin LTO module within the bitcode file (this will presumably need to be used to ignore the regular LTO summaries when importing etc during runThinLTO)
2) Call computeDeadSymbols earlier (from LTO::run instead of LTO::runThinLTO)
3) Teach runRegularLTO to internalize the resulting dead symbols on the regular LTO module
4) Figure out how not to set VisibleOutsideThinLTO when the reference is from the companion regularLTO module in the multi-module bitcode file (so that the symbol in this case isn't mistakenly considered escaped and therefore a 
GUIDPreservedSymbol when we invoke computeDeadSymbols).

I think we may also need to consider the case where the bitcode file contains only a regular module (i.e. the case where the module contains type metadata and does not define external symbols). In that case we may want a standalone def/use "summary" for the regular module so that it can participate in dead stripping.

Not sure though -- my naive expectation is that all of that module's uses would typically be GC roots anyway.

 Issue 697485  has been merged into this issue.

I have posted a workaround for SwiftShader: https://swiftshader-review.googlesource.com/#/c/9308/ as suggested by Teresa in #7.

Could anyone clarify whether this is a bug in SwiftShader's code or build files, or if this is a shortcoming of ThinLTO which you intend to fix? The description in https://swiftshader-review.googlesource.com/#/c/9308/1//COMMIT_MSG is a bit cryptic to me. Thanks.

The root cause is a bug in swiftshader -- it should not require optimizations (in this case, dead code removal in the linker) to be enabled in order to link correctly.

Implementing the linker optimizations for ThinLTO is something that we would want to implement anyway, and as a side effect it would "fix" this issue, but ideally swiftshader should not depend on it.

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/628a8496e44ec8a096125cb0cdc3e63bb7dd8380

commit 628a8496e44ec8a096125cb0cdc3e63bb7dd8380
Author: Ivan Krasin <krasin@chromium.org>
Date: Tue Apr 18 20:38:37 2017

Fix SwiftShader's libEGL build under ThinLTO.

Linking libEGL.so of the SwiftShader's flavor requires
the linker to be too smart (there's a heavy reliance on
garbage collecting unused symbols, for which there is
no guarantees in the general case). In ThinLTO case,
garbage collection is still not as sofisticated, as in
other cases, so it fails to link the target complaining
about undefined symbols.

The workaround is to prevent Clang from splitting the
bitcode files in the question into two, and making the
GC problem a bit easier.

Eventually, ThinLTO might get a better GC, but it might
be a good idea to not rely on this feature in the source
code. At least, no other targets in Chromium do.

BUG= chromium:686980 

Change-Id: Ib44f65c4825cc3f6cd24695738a71ca4661f0bfb
Reviewed-on: https://swiftshader-review.googlesource.com/9308
Tested-by: Ivan Krasin <krasin@chromium.org>
Reviewed-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/628a8496e44ec8a096125cb0cdc3e63bb7dd8380/src/OpenGL/libEGL/BUILD.gn

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/158dcfc13fe3e36217f1bf0d4ccbd8bd80f6d5f1

commit 158dcfc13fe3e36217f1bf0d4ccbd8bd80f6d5f1
Author: Ivan Krasin <krasin@chromium.org>
Date: Tue Apr 18 20:38:54 2017

Disable CFI on SwiftShader's libEGL.

BUG= chromium:686980 

Change-Id: I0224093fbbffb2bac8a84c95a8370617c4df6978
Reviewed-on: https://swiftshader-review.googlesource.com/9309
Tested-by: Ivan Krasin <krasin@chromium.org>
Reviewed-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/158dcfc13fe3e36217f1bf0d4ccbd8bd80f6d5f1/src/OpenGL/libEGL/BUILD.gn

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/ad675fa1d6617ff7fe5965845152ba71f33caf61

commit ad675fa1d6617ff7fe5965845152ba71f33caf61
Author: Ivan Krasin <krasin@chromium.org>
Date: Tue Apr 18 20:39:02 2017

Also fix libGLESv2.

BUG= chromium:686980 

Change-Id: I5f22f57288246c262a0fdb0ca1b0b321e4a58c1b
Reviewed-on: https://swiftshader-review.googlesource.com/9310
Tested-by: Ivan Krasin <krasin@chromium.org>
Reviewed-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/ad675fa1d6617ff7fe5965845152ba71f33caf61/src/OpenGL/libGLESv2/BUILD.gn

I have just merged the workaround. But yes, fixing the SwiftShader's code will be a much better solution.

Thanks for clarifying that. I've filed  Issue swiftshader:46  to look into it. There's an awkward dependency cycle between libEGL, libGLES_CM, and libGLESv2 that's at the root of all this. My current design works for Windows, Linux, Mac OS, and Android, but apparently it's still quite fragile.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c9bd5f0202de1500d75c22654b2e22f0bc34312

commit 7c9bd5f0202de1500d75c22654b2e22f0bc34312
Author: capn <capn@chromium.org>
Date: Wed Apr 19 17:28:33 2017

Roll SwiftShader 30385f0..dc7759c

https://swiftshader.googlesource.com/SwiftShader.git/+log/30385f0..dc7759c
- Fixes ThinLTO linking.
- Fixes TSAN data race.

BUG= chromium:686980 ,  chromium:710753 

TBR=kbr@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2826133002
Cr-Commit-Position: refs/heads/master@{#465654}

[modify] https://crrev.com/7c9bd5f0202de1500d75c22654b2e22f0bc34312/DEPS

I've fixed  Issue swiftshader:31 , which may allow removing the ThinLTO-specific workaround. Unfortunately I can't seem to reproduce the original issue. The GN argument 'llvm_force_head_revision = true' produces a Python error so I commented that out, but I assume it's essential and goes along with a custom Clang build with 'clang_base_path = "redacted2/llvm-project/ra"'?

krasin@, could you try locally reverting the changes you made to SwiftShader's build files (on the latest revision), and check if the problem still occurs?

krasin's left. pcc, could you check?

The patch landed in LLVM trunk and was rolled into our copy of clang/lld so it should no longer require a custom clang.

I reverted krasin's changes locally and was unable to reproduce the problem. So it seems that we should be able to revert them now.

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/3e88aaf80b2965954638be88c29175aa34fc657a

commit 3e88aaf80b2965954638be88c29175aa34fc657a
Author: Nicolas Capens <capn@google.com>
Date: Wed Jun 21 21:05:09 2017

Revert workarounds for ThinLTO linker builds.

Fixing  Issue swiftshader:31  makes the workarounds obsolete and since
they disabled some link-time optimizations they should be reverted.

This reverts commit 628a8496e44ec8a096125cb0cdc3e63bb7dd8380.
This reverts commit 158dcfc13fe3e36217f1bf0d4ccbd8bd80f6d5f1.
This reverts commit ad675fa1d6617ff7fe5965845152ba71f33caf61.

 Bug chromium:686980 
Bug chromium:735508

Change-Id: I4c4e2466fdfcddd12854b87e5a62d6ec8c823a05
Reviewed-on: https://swiftshader-review.googlesource.com/10168
Reviewed-by: Nicolas Capens <capn@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/3e88aaf80b2965954638be88c29175aa34fc657a/src/OpenGL/libEGL/BUILD.gn
[modify] https://crrev.com/3e88aaf80b2965954638be88c29175aa34fc657a/src/OpenGL/libGLESv2/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df3cf28204ad1102ca9c8d79182d46dc44011c64

commit df3cf28204ad1102ca9c8d79182d46dc44011c64
Author: capn <capn@chromium.org>
Date: Sat Jun 24 00:15:49 2017

Roll SwiftShader 3b9e1ea..9282c6d

https://swiftshader.googlesource.com/SwiftShader.git/+log/3b9e1ea..9282c6d

BUG=735508
BUG= 686980 
BUG= 732667 

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Change-Id: I44ad5b4094a66e5e90bb354d9563be61d6b92623
Review-Url: https://codereview.chromium.org/2953313002
Cr-Commit-Position: refs/heads/master@{#482085}

[modify] https://crrev.com/df3cf28204ad1102ca9c8d79182d46dc44011c64/DEPS

Adding a trybot to prevent this from happening again and close this off: https://chromium-review.googlesource.com/547031

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/997b71924f38eb831ba87cf97a07371100715268

commit 997b71924f38eb831ba87cf97a07371100715268
Author: Nicolas Capens <capn@google.com>
Date: Tue Jun 27 14:47:54 2017

Add CFI trybot to SwiftShader DEPS roll script.

The linux_chromium_cfi_rel_ng (Control-Flow Integrity) trybot builds
with ThinLTO enabled. This should prevent future regressions during
DEPS rolls.

BUG= 686980 

Change-Id: Id7b19f0d941f9027f4914e6f2ecf88fb0fee7759
Reviewed-on: https://chromium-review.googlesource.com/547031
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nicolas Capens <nicolascapens@google.com>
Cr-Commit-Position: refs/heads/master@{#482625}
[modify] https://crrev.com/997b71924f38eb831ba87cf97a07371100715268/tools/roll_swiftshader.py