Can you try making the following changes:
- remove any lines referring to -fwhole-program-vtables in build/
- add this line to tools/cfi/blacklist.txt in the "Cross-DSO vcalls" section:
type:icu::*

If that works, we will probably want to change the unbundle script to do something similar.

Unfortunately Chromium still crashed at the same spot.

Next thing to try:

nm -D /usr/lib/x86_64-linux-gnu/libicui18n.so | c++filt # or wherever this .so is on your machine

some of the symbols will be named icu_[some number]::something

Change the line in the blacklist to

type:icu_[some number]::*

You're probably right about that. I did try blacklisting "type:icu_60::*" at some point but my use of ccache must have resulted in the same object files as with "type:icu::*". I'll do a clean build to verify this.

On the bright side, I also built Chromium with '-fno-sanitize-trap=cfi-vcall -fsanitize-recover=cfi-vcall' and only ICU failures were logged (though I didn't poke around Chromium too much). The following types were affected:

  icu_60::BreakIterator
  icu_60::Collator
  icu_60::Normalizer2
  icu_60::TimeZone
  icu_60::Transliterator
  icu_60::UnicodeSet

Deleted: cfi-diagnostics.txt 16.1 KB

cfi-diagnostics.txt 16.1 KB View Download

And indeed blacklisting "type:icu_60::*" works. :)

Just curious, why does -fwhole-program-vtables need to be removed? I did another build with only the above blacklist addition and it seems to run fine as well (and the binary is 1.3 MiB smaller).

Would something like this make sense?

https://chromium-review.googlesource.com/c/chromium/src/+/966907

The issue seems to be out of TE-scope as it seems to be related to building chromium. Hence, adding label TE-NeedsTriageHelp for further investigation from dev team.

Thanks...!!

> Just curious, why does -fwhole-program-vtables need to be removed?

This flag allows the compiler to apply optimizations that assume that all C++ vtables are visible at (static) link time. This is the case in regular builds of Chromium, but not when a C++ library is unbundled.

It's entirely possible that the browser would seem to start up fine with this flag but it may lead to mysterious crashes or other misbehaviour in some circumstances.

Thank you for the explanation! I didn't realize it was so restrictive. It seems weird to me that -fwhole-program-vtables can't be used when linking to shared C++ libraries. I'd have guessed that the only requirement would be to compile everything with LTO and that it'd play nice with any system libs.

I understand that I cannot, for example, build Chromium and ICU separately with -fwhole-program-vtables. What I'm having trouble understanding is why whole-program vtable optimization cannot be used just on Chromium in Linux distro builds where it links to system ICU and libstdc++. 

My very basic understanding is that the class hierarchy is known and won't be extended further since Chromium is the final linked program. ICU doesn't call into Chromium either, so there won't be any breakage there. What am I missing?

I tried going through your change that introduced whole-program optimization to LLVM but unfortunately it's over my head (https://reviews.llvm.org/D16795).

When I said "This flag allows the compiler to apply optimizations that assume that all C++ vtables are visible at (static) link time." I misspoke. What I meant to say was that the flag only allows the compiler to make these assumptions about classes which do not have public LTO visibility (which approximately means __attribute__((visibility("default"))); see https://clang.llvm.org/docs/LTOVisibility.html for an exact definition). This is exactly to handle cases where classes are defined in external DSOs (such as libstdc++ or ICU).

It looks like ICU will declare its classes with default visibility if you build without U_STATIC_IMPLEMENTATION defined. The compiler will also disable CFI for such classes. So I suppose that an alternative solution would be to arrange for U_STATIC_IMPLEMENTATION not to be defined when building Chromium. That should allow you to keep -fwhole-program-vtables enabled.

It should also allow you to remove the entries from the blacklist because they would be implied by public LTO visibility.

ICU's platform.h has this:

  #define U_EXPORT __attribute__((visibility("default")))

But it doesn't get applied to any of the methods, except for this (in stringpiece.h):

  U_EXPORT UBool U_EXPORT2
  operator==(const StringPiece& x, const StringPiece& y) {

libstdc++, on the other hand, applies public visibility at the namespace level.

I can't imagine the lack of explicit public visibility being an unintentional omission in ICU's headers.

Again, thank you for helping me understand what's going on. Your approach to specify public LTO visibility to icu::* classes sounds ideal to me; I'm just not sure how it can be implemented cleanly if ICU doesn't already have a setting for this.

Perhaps (re)defining U_NAMESPACE_BEGIN to include a visibility attribute could work?

> Perhaps (re)defining U_NAMESPACE_BEGIN to include a visibility attribute could work?

Scratch that. U_NAMESPACE_BEGIN is not something that can be set outside of uversion.h; the latter defines it unconditionally. :\

Taking a better look at ICU's headers, U_EXPORT does seem to get applied to the classes through the U_*_API defines in utypes.h. I think I might need to define U_COMBINED_IMPLEMENTATION; I'll do some testing to confirm.

Darn it, with U_COMBINED_IMPLEMENTATION defined, ICU forces the type of UChar to be char16_t, which prevents the "UCHAR_TYPE=uint16_t" workaround in unbundle/icu.gn to take effect. [1]

[1] https://chromium.googlesource.com/chromium/src.git/+/92e81420%5E%21/

Maybe you could try defining U_IMPORT to __attribute__((visibility("default"))) and not defining any of U_*_IMPLEMENTATION so that you end up choosing this code path: https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/utypes.h?type=cs&q=%5Cbu_combined_implementation%5Cb&sq=package:chromium&l=362

ICU should probably be defining U_IMPORT like that anyway.

Indeed, that code path seems like a good way to implement a short-term workaround. I chose the slightly lazier way of appending "U_IMPORT=U_EXPORT" to the defines in unbundle/icu.gn (under the "UCHAR_TYPE=uint16_t").

Seems to work, but I'll have to do a clean build to make sure. My understanding from your advice so far is that this would set public visibility for the ICU classes (and thus public LTO visibility) and should allow enabling is_cfi in combination with -fwhole-program-vtables. (Please correct me if I'm wrong.)

Long term, after Chromium switches the type of UChar to char16_t, I think the correct adjustment to unbundle/icu.gn would be to define U_COMMON_IMPLEMENTATION and U_I18N_IMPLEMENTATION.

> Long term, after Chromium switches the type of UChar to char16_t, I think the correct adjustment to unbundle/icu.gn would be to define U_COMMON_IMPLEMENTATION and U_I18N_IMPLEMENTATION.

Or maybe U_COMBINED_IMPLEMENTATION. It's a bit unclear what happens if a target depends on both icuuc and icui18n; even with separate U_{COMMON,I18N}_IMPLEMENTATION definitions for each library, wouldn't the first definition cause the other's U_*_API to be set to U_IMPORT?

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

commit f15e8b573ada0fcd643ae393484214b1c7c940f8
Author: Evangelos Foutras <evangelos@foutrelis.com>
Date: Sat Mar 24 00:04:33 2018

Fix crash in is_cfi=true builds with unbundled ICU

Ensure ICU symbols have public visibility and are thus excluded from CFI
checks and whole-program optimization. The former caused a startup crash
and the latter has the potential to break virtual calls in weird ways.

BUG= 822820 

Change-Id: Ia809eefcb9e93b3c612f2381d394db83bbc67120
Reviewed-on: https://chromium-review.googlesource.com/978008
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545638}
[modify] https://crrev.com/f15e8b573ada0fcd643ae393484214b1c7c940f8/build/linux/unbundle/icu.gn

This should be fixed by the above commit; thanks again Peter for your help in figuring out how to address this.

I'll try adding this CL and is_cfi=true to the Arch Linux Chromium package and report back if any issues arise.

Please mark this as fixed. :)

No problem at all. As an Arch Linux user myself I also wanted to thank you for enabling CFI in the Arch package :)