Don't we have monitoring for this? That roll landed more than a month ago.

Maybe related to  issue 872926 ? That was also about sizes and in the same time frame.

eyaich@: do you know if this is still being monitored? I'm not sure whether I'm just failing to find the right graph.

re: #c1

I thought we had monitoring for ChromiumPerf/mac/sizes, but it's stopped updating. There's ChromiumPerf/chromium_perf/sizes instead. Those are both hard to navigate - if the range crosses the switchover you get "filed to fetch graph data". 

There's also ChromiumChrome/Google Chrome Mac/sizes which has continuous data and is at the bottom of the link in the description.

Maybe sullivan@ knows what's migrating where, or jbudorick? (e.g. Issue 828650?). Or Issue 758326? Or Issue 828468? Or Issue 836798? Ah, oh - I found a doc - go/sdvzd --> eyaich@

+simonhatch who manages the monitoring and migration of perf dashboard graphs

Not really clear when/if this was migrated from somewhere else. The sheriff config contains these lines:

ChromiumPerf/mac/sizes/GoogleChrome.app
ChromiumPerf/mac/sizes/GoogleChromeFramework

If you want these to point at chromium_perf or anywhere else, let me know and I can make the change.

Where does that config live?

I'm pretty sure ChromiumPerf/mac/sizes/* is no longer updating, so those lines are now ineffective. There may be monitoring of other things also lost (i.e. not just sizes). I've filed  Issue 892460  for the missing monitoring, so we can focus on the clang thing here.

That config lives on the dashboard, it's admin only, so filing a bug was the right way to go.

Looking at this today.

Google Chrome Framework.framework/Versions/A/Google Chrome Framework (130900 to 133116 KB)

but also

Google Chrome Framework.framework/Versions/A/Libraries/WidevineCdm/_platform_specific/mac_x64/libwidevinecdm.dylib (7064 to 7776 KB).

(I built just before and after the roll, with these gn args:

is_chrome_branded = true
is_debug = false
is_official_build = true
strip_absolute_paths_from_debug_symbols = true
use_goma = true)


This is the bloaty diff:

$ /work/bloaty/bloaty out/release.bad/Google\ Chrome\ Framework.framework/Versions/A/Google\ Chrome\ Framework -- out/release.good/Google\ Chrome\ Framework.framework/Versions/A/Google\ Chrome\ Framework
     VM SIZE                                 FILE SIZE
 --------------                           --------------
 +51e4%  +798Ki Weak Binding Info          +798Ki +51e4%
 +12e2%  +788Ki String Table               +788Ki +12e2%
 +22e4%  +486Ki Export Info                +486Ki +22e4%
  +224% +91.7Ki Symbol Table              +91.7Ki  +224%
  +213% +45.9Ki __DATA,__got              +45.9Ki  +213%
   +94% +22.9Ki Indirect Symbol Table     +22.9Ki   +94%
  +0.2%     +64 __TEXT,__eh_frame             +64  +0.2%
  +0.0%     +48 __DATA,__data                 +48  +0.0%
  -0.0%      -8 Table of Non-instructions      -8  -0.0%
  -0.0%     -32 __TEXT,__gcc_except_tab       -32  -0.0%
  -0.0%     -80 Function Start Addresses      -80  -0.0%
  -0.1%     -88 Rebase Info                   -88  -0.1%
 -71.1%    -128 [__DATA]                     -128 -19.0%
  -0.0%    -144 __TEXT,__cstring             -144  -0.0%
  -0.0%    -180 __TEXT,__unwind_info         -180  -0.0%
  -0.0%    -224 __TEXT,__const               -224  -0.0%
  -0.0% -1.80Ki __DATA,__const            -1.80Ki  -0.0%
 -58.0% -2.40Ki [__TEXT]                  -2.40Ki -58.4%
  -0.0% -13.1Ki __TEXT,__text             -13.1Ki  -0.0%
  +1.7% +2.16Mi TOTAL                     +2.16Mi  +1.7%

Looking at exported functions from the .dylib (hopefully I'm doing this correctly) before the roll:

$ nm -gU out/release.good/Google\ Chrome\ Framework.framework/Versions/A/Google\ Chrome\ Framework        
00000000079ac268 S _CIDetectorTypeQRCode
00000000079ac280 S _CIDetectorTypeText
0000000000002e00 T _ChromeAppModeStart_v4
0000000000004440 T _ChromeMain
0000000000003d30 T _KeychainReauthorizeIfNeededAtUpdate
0000000007e71c28 S _OBJC_CLASS_$_RTCDispatcher
0000000007e71c00 S _OBJC_METACLASS_$_RTCDispatcher

(That's 7 symbols)

After the roll:

$ nm -gU out/release.bad/Google\ Chrome\ Framework.framework/Versions/A/Google\ Chrome\ Framework | head -35
00000000079b38c8 S _CIDetectorTypeQRCode
00000000079b38e0 S _CIDetectorTypeText
0000000000002400 T _ChromeAppModeStart_v4
0000000000003a40 T _ChromeMain
0000000000003330 T _KeychainReauthorizeIfNeededAtUpdate
0000000007e78c78 S _OBJC_CLASS_$_RTCDispatcher
0000000007e78c50 S _OBJC_METACLASS_$_RTCDispatcher
0000000007ec4428 D __ZGRZNK5blink11CSSLonghand10AlignItems27GetPropertyNameAtomicStringEvE4name_
0000000007ec49a0 D __ZGRZNK5blink11CSSLonghand10BreakAfter27GetPropertyNameAtomicStringEvE4name_
0000000007ec4a18 D __ZGRZNK5blink11CSSLonghand10CaretColor27GetPropertyNameAtomicStringEvE4name_
0000000007ec4b08 D __ZGRZNK5blink11CSSLonghand10ColumnFill27GetPropertyNameAtomicStringEvE4name_
0000000007ec4b80 D __ZGRZNK5blink11CSSLonghand10ColumnSpan27GetPropertyNameAtomicStringEvE4name_
0000000007ec4cb8 D __ZGRZNK5blink11CSSLonghand10EmptyCells27GetPropertyNameAtomicStringEvE4name_
0000000007ec4d78 D __ZGRZNK5blink11CSSLonghand10FlexShrink27GetPropertyNameAtomicStringEvE4name_
0000000007ec4dc0 D __ZGRZNK5blink11CSSLonghand10FloodColor27GetPropertyNameAtomicStringEvE4name_
0000000007ec4df0 D __ZGRZNK5blink11CSSLonghand10FontFamily27GetPropertyNameAtomicStringEvE4name_
0000000007ec4f10 D __ZGRZNK5blink11CSSLonghand10FontWeight27GetPropertyNameAtomicStringEvE4name_
0000000007ec4fa0 D __ZGRZNK5blink11CSSLonghand10GridRowEnd27GetPropertyNameAtomicStringEvE4name_
0000000007ec5078 D __ZGRZNK5blink11CSSLonghand10InlineSize27GetPropertyNameAtomicStringEvE4name_
0000000007ec51b0 D __ZGRZNK5blink11CSSLonghand10LineHeight27GetPropertyNameAtomicStringEvE4name_
0000000007ec52a0 D __ZGRZNK5blink11CSSLonghand10MarginLeft27GetPropertyNameAtomicStringEvE4name_
0000000007ec54b0 D __ZGRZNK5blink11CSSLonghand10OffsetPath27GetPropertyNameAtomicStringEvE4name_
0000000007ec56d8 D __ZGRZNK5blink11CSSLonghand10PaddingTop27GetPropertyNameAtomicStringEvE4name_
0000000007ec5708 D __ZGRZNK5blink11CSSLonghand10PaintOrder27GetPropertyNameAtomicStringEvE4name_
0000000007ec5c30 D __ZGRZNK5blink11CSSLonghand10TextAnchor27GetPropertyNameAtomicStringEvE4name_
0000000007ec5cc0 D __ZGRZNK5blink11CSSLonghand10TextIndent27GetPropertyNameAtomicStringEvE4name_
0000000007ec5d38 D __ZGRZNK5blink11CSSLonghand10TextShadow27GetPropertyNameAtomicStringEvE4name_
0000000007ec5eb8 D __ZGRZNK5blink11CSSLonghand10UserSelect27GetPropertyNameAtomicStringEvE4name_
0000000007ec5f18 D __ZGRZNK5blink11CSSLonghand10Visibility27GetPropertyNameAtomicStringEvE4name_
0000000007ec64a0 D __ZGRZNK5blink11CSSLonghand10WhiteSpace27GetPropertyNameAtomicStringEvE4name_
0000000007ec64e8 D __ZGRZNK5blink11CSSLonghand10WillChange27GetPropertyNameAtomicStringEvE4name_
0000000007ec49b8 D __ZGRZNK5blink11CSSLonghand11BreakBefore27GetPropertyNameAtomicStringEvE4name_
0000000007ec49d0 D __ZGRZNK5blink11CSSLonghand11BreakInside27GetPropertyNameAtomicStringEvE4name_
0000000007ec4a00 D __ZGRZNK5blink11CSSLonghand11CaptionSide27GetPropertyNameAtomicStringEvE4name_
0000000007ec4af0 D __ZGRZNK5blink11CSSLonghand11ColumnCount27GetPropertyNameAtomicStringEvE4name_
[snip]

(That's 5875 symbols)

I see a ton of symbols from blink, but also v8, IPC, network, etc.

So this doesn't seem related to the libc++ thing we hit on iOS, but more like something changed in Clang regarding symbol visibility?

Looking at one of the symbols further down in the output:

__ZZNK7network17ResourceScheduler6Client25RecordRequestCountMetricsEvE24atomic_histogram_pointer

demangler.com shows it as:

_network::ResourceScheduler::Client::RecordRequestCountMetrics() const::atomic_histogram_pointer

Does that mean it's a static local maybe?

Anyway, it comes from services/network/resource_scheduler.cc

Let's try to bisect the Clang roll range and see if the symbol table changes...

Before and after the roll the linkage has gone from:

llvm-nm -m out/release.good/obj/services/network/network_service/resource_scheduler.o | grep __ZZNK7network17ResourceScheduler6Client25RecordRequestCountMetricsEvE24atomic_histogram_pointer\\b
0000000000003928 (__DATA,__data) weak private external __ZZNK7network17ResourceScheduler6Client25RecordRequestCountMetricsEvE24atomic_histogram_pointer

to:

llvm-nm -m out/release.bad/obj/services/network/network_service/resource_scheduler.o | grep __ZZNK7network17ResourceScheduler6Client25RecordRequestCountMetricsEvE24atomic_histogram_pointer\\b
0000000000003908 (__DATA,__data) weak external __ZZNK7network17ResourceScheduler6Client25RecordRequestCountMetricsEvE24atomic_histogram_pointer

Bisection points to this:
---
Author: inouehrs
Date: Tue Aug 21 22:43:27 2018
New Revision: 340386

URL: http://llvm.org/viewvc/llvm-project?rev=340386&view=rev
Log:
[AST] correct the behavior of -fvisibility-inlines-hidden option (don't make static local variables hidden)

The command line option -fvisibility-inlines-hidden makes inlined method hidden, but it is expected not to affect the visibility of static local variables in the function.
However, Clang makes the static local variables in the function also hidden as reported in PR37595. This problem causes LLVM bootstarp failure on Fedora 28 if configured with -DBUILD_SHARED_LIBS=ON.

This patch makes the behavior of -fvisibility-inlines-hidden option to be consistent with that of gcc; the option does not change the visibility of the static local variables if the containing function does not associated with explicit visibility attribute and becomes hidden due to this option.

Differential Revision: https://reviews.llvm.org/D50968
---

Guess we want -fvisibility-inlines-hidden=2 or something to get the old behavior :-/

+rnk who reviewed that patch

We could call it -fvisibility-inline-static-locals-hidden :-P

But the old behaviour would have caused problems if we'd linked against some system library with an inline static local that was used both in our binary and the system's, as that was the motivation for the patch.

So I'm not sure what we really do want actually. Something like "stuff coming from our own source files is hidden by default"?

Also, I suppose this doesn't hit us on Linux because we build a static binary, and not on Windows because dllexport is different, but wouldn't this have regressed the binary size on Android, which also uses a shared library?

On Android we use a linker script to hide all symbols (besides JNI). I'd guess this change would maybe still hurt compile-time optimization of affected symbols, but wouldn't cause extra exports in the end.

agrieve: Cool, thanks.

Maybe just exporting symbols explicitly is what we want for Mac too? Nico, didn't we use to do this before?

Not as far as I know. But on mac, we have this file which is supposed to verify our list of exports: https://cs.chromium.org/chromium/src/chrome/app/framework.order?q=file:%5C.order&sq=package:chromium&dr

...oh, looks like this is used as order file (https://chromium.googlesource.com/chromium/src/+/e8b24a9c1528052fc510b17945822c906187b72d) not as verification step after all. +rsesek who probably knows what's supposed to be happening there.

But the "static variable in system library" is really a linux-only problem, and rare even there. No sane platform has C++ in its header files or statics in system headers. So that patch seems like a workaround for broken platform headers, and I'm not sure what I feel about making everyone pay the price for that :-/

Another thought: this isn't really a shared-library build, the compile command looks like this:

clang++ -MMD -MF obj/services/network/network_service/resource_scheduler.o.d -DIS_NETWORK_SERVICE_IMPL -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DOFFICIAL_BUILD -DGOOGLE_CHROME_BUILD -D_LIBCPP_HAS_NO_ALIGNED_ALLOCATION -D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN\ _LIBCPP_ALWAYS_INLINE -DCR_XCODE_VERSION=0832 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DWEBP_EXTERN=extern -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -DWEBRTC_NON_STATIC_TRACE_EVENT_HANDLERS=0 -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DWEBRTC_MAC -DABSL_ALLOCATOR_NOTHROW=1 -DNO_MAIN_THREAD_WRAPPING -DXML_STATIC -I../.. -Igen -I../../third_party/libwebp/src -I../../third_party/libyuv/include -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/webrtc_overrides -I../../third_party/webrtc -I../../third_party/abseil-cpp -I../../third_party/webrtc_overrides -I../../testing/gtest/include -I../../third_party/webrtc -I../../third_party/expat/files/lib -fno-strict-aliasing -fstack-protector -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -arch x86_64 -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wunguarded-availability -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-user-defined-warnings -Wno-unused-lambda-capture -Wno-null-pointer-arithmetic -Wno-enum-compare-switch -Wno-ignored-pragma-optimize -O2 -fno-omit-frame-pointer -gdwarf-2 -fno-standalone-debug -isysroot ../../build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -mmacosx-version-min=10.9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -std=c++14 -stdlib=libc++ -fno-exceptions -fno-rtti -fvisibility-inlines-hidden -c ../../services/network/resource_scheduler.cc -o obj/services/network/network_service/resource_scheduler.o


I.e. it's got both -fvisibility=hidden and -fvisibility-inlines-hidden

And I don't see COMPONENT_BUILD getting defined, so there should be no visibility annotations on the class.

Shouldn't -fvisibility=hidden mean that everything's hidden, including these static locals, unless there is an attribute on the class or function to set default visibility?

We do have a script (https://cs.chromium.org/chromium/src/chrome/tools/build/mac/verify_order?q=verify_order&sq=package:chromium&dr) that ensures that _ChromeMain is the last exported symbol in the binary: https://cs.chromium.org/chromium/src/chrome/BUILD.gn?l=1326&rcl=e85ee524dbfcdc8a26d7fcf16a6010ee633be45f. The framework.order file is passed as -Wl,-order_file when building the Framework.

So the listing in #10 seems wrong for two reasons: _KeychainReauthorizeIfNeededAtUpdate is coming after _ChromeMain (against the framework.order file), and there are all those extra symbols.

> Shouldn't -fvisibility=hidden mean that everything's hidden, including these static locals, unless there is an attribute on the class or function to set default visibility?

Yes, it seems Clang and GCC behave differently here:

inline int foo() {
  static int x;
  return x++;
}

int use() { return foo(); }


$ g++ -fvisibility=hidden -fvisibility-inlines-hidden -c /tmp/a.cc && objdump -t a.o

a.o:     file format elf64-x86-64

SYMBOL TABLE:
0000000000000000 l    df *ABS*  0000000000000000 a.cc
0000000000000000 l    d  .text  0000000000000000 .text
0000000000000000 l    d  .data  0000000000000000 .data
0000000000000000 l    d  .bss   0000000000000000 .bss
0000000000000000 l    d  .bss._ZZ3foovE1x       0000000000000000 .bss._ZZ3foovE1x
0000000000000000 l    d  .text._Z3foov  0000000000000000 .text._Z3foov
0000000000000000 l    d  .note.GNU-stack        0000000000000000 .note.GNU-stack
0000000000000000 l    d  .eh_frame      0000000000000000 .eh_frame
0000000000000000 l    d  .comment       0000000000000000 .comment
0000000000000000 l    d  .group 0000000000000000 .group
0000000000000000 l    d  .group 0000000000000000 .group
0000000000000000 u     O .bss._ZZ3foovE1x       0000000000000004 .hidden _ZZ3foovE1x   <---- foo()::x is hidden
0000000000000000  w    F .text._Z3foov  0000000000000015 .hidden _Z3foov
0000000000000000 g     F .text  000000000000000b .hidden _Z3usev


$ clang++ -fvisibility=hidden -fvisibility-inlines-hidden -c /tmp/a.cc && objdump -t a.o

a.o:     file format elf64-x86-64

SYMBOL TABLE:
0000000000000000 l    df *ABS*  0000000000000000 a.cc
0000000000000000 l    d  .text  0000000000000000 .text
0000000000000000 l    d  .text._Z3foov  0000000000000000 .text._Z3foov
0000000000000000  w    F .text._Z3foov  0000000000000019 .hidden _Z3foov
0000000000000000 g     F .text  000000000000000b .hidden _Z3usev
0000000000000000  w    O .bss._ZZ3foovE1x       0000000000000004 _ZZ3foovE1x    <----- foo()::x is not hidden

I agree with what I've read so far: It's a clang bug for static locals in inline functions to get default visibility when -fvisibility=hidden and -fvisibility-inlines-hidden are set.

As a workaround, why bother setting -fvisibility-inlines-hidden and not just -fvisibility=hidden? I'm pretty sure -fvisibility=hidden covers inline functions anyway. The flag to make inline functions hidden only really exists as an optimization for people who can't use -fvisibility=hidden because they don't have explicit annotations or a separate export list.

Patch: https://reviews.llvm.org/D53052
Upstream bug: https://bugs.llvm.org/show_bug.cgi?id=39236

I expect we'll pick up the fix in the next roll.

> why bother setting -fvisibility-inlines-hidden and not just -fvisibility=hidden? I'm pretty sure -fvisibility=hidden covers inline functions anyway. The flag to make inline functions hidden only really exists as an optimization for people who can't use -fvisibility=hidden because they don't have explicit annotations or a separate export list.

I think setting both flags is useful when you want hidden visibility in general, but you give some classes default visibility with an attribute, and still want inline members to be hidden as an optimization. I think that's what Chromium is going for.

I.e.:

  struct __attribute__((visibility("default"))) S {
    int foo() { return 42; }
    int bar();
  };
  int S::bar() { return 45; }
  void use(S *s) { s->foo(); }

Using both flags, S::bar() has default visibility, but S::foo() is still hidden.

Reid's fix landed in Clang r344190, so this should be fixed once we roll past that.

I was building locally to confirm the binary size is back to normal. But with Reid's change I get new linker warnings.
(from the libswiftshader_libGLESv2.dylib target)

ld: warning: direct access in function 'es2::Texture3D::setImage(int, int, int, int, int, unsigned int, unsigned int, gl::PixelStorageModes const&, void const*)'
from file 'obj/third_party/swiftshader/src/OpenGL/libGLESv2/libswiftshader_libGLESv2_static.a(Texture.o)'
to global weak symbol 'es2::ImageLevels::operator[](unsigned long)::nullImage'
from file 'obj/third_party/swiftshader/src/OpenGL/libGLESv2/libswiftshader_libGLESv2_static.a(Texture.o)'
means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
[...and other references to the same variable]


Texture.o is compiled like this:

clang++ -MMD -MF obj/third_party/swiftshader/src/OpenGL/libGLESv2/swiftshader_libGLESv2_static/Texture.o.d -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DOFFICIAL_BUILD -DGOOGLE_CHROME_BUILD -D_LIBCPP_HAS_NO_ALIGNED_ALLOCATION -DCR_XCODE_VERSION=0832 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -DNO_SANITIZE_FUNCTION=__attribute__\(\(no_sanitize\(\"function\"\)\)\) -DANGLE_DISABLE_TRACE -DNDEBUG -DGL_API= -DGL_GLEXT_PROTOTYPES -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -DGLAPI=GL_APICALL -DGL_APICALL=__attribute__\(\(no_sanitize\(\"function\"\)\)\) -I../../third_party/swiftshader/include -I../../third_party/swiftshader/src -I../../third_party/swiftshader/src/OpenGL -I../.. -Igen -fno-strict-aliasing -fstack-protector -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -arch x86_64 -O2 -fno-omit-frame-pointer -gdwarf-2 -fno-standalone-debug -isysroot ../../build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -mmacosx-version-min=10.9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Werror -Wall -Wno-unused-variable -Wunguarded-availability -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-user-defined-warnings -Wno-unused-lambda-capture -Wno-null-pointer-arithmetic -Wno-enum-compare-switch -Wno-ignored-pragma-optimize -Wno-defaulted-function-deleted -std=c++11 -fno-exceptions -fno-operator-names -ffunction-sections -fdata-sections -fomit-frame-pointer -Os -m64 -fPIC -march=x86-64 -mtune=generic -Wno-sign-compare -fvisibility=protected -std=c++14 -stdlib=libc++ -fno-exceptions -fno-rtti -fvisibility-inlines-hidden -c ../../third_party/swiftshader/src/OpenGL/libGLESv2/Texture.cpp -o obj/third_party/swiftshader/src/OpenGL/libGLESv2/swiftshader_libGLESv2_static/Texture.o


I.e. -fvisibility=hidden -fvisibility=protected -fvisibility-inlines-hidden. Quite a combo :-)

I believe this means that while es2::ImageLevels::operator[](unsigned long) is hidden, the nullImage static local is protected, i.e. it's visible from the outside but cannot be interposed and so is not accessed through the GOT and that's what the linker means by "accessed directly".

It sounds like this is actually working as intended though, and the linker is just being overly nervous?

But hang on, what if someone else were to build a shared library with -fvisibility=protected and #include the inline function with static local from swiftshader. Would it now reference a different definition than the swiftshader one? This "protected" visibility sounds scary..

(nullImage is silly anyway, it's always NULL so the code could be fixed to avoid this linker warning...)



We get the same warning from this target:

[9176/35712] SOLINK libswiftshader_libEGL.dylib [...]
warning: direct access in function 'getModuleDirectory()'
from file 'obj/third_party/swiftshader/src/OpenGL/libEGL/swiftshader_libEGL/Display.o'
to global weak symbol 'getModuleDirectory()::dummy_symbol'
from file 'obj/third_party/swiftshader/src/OpenGL/libEGL/swiftshader_libEGL/Display.o'
means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.

Here the local variable is used as an address to pass to dlsym. I think this still works fine though.


Reid: does this make any sense to you?

Before:
-rwxr-xr-x  3 hwennborg  wheel  136914068 Oct 11 13:09 out/release.344189/Google Chrome Framework.framework/Versions/A/Google Chrome Framework

After:
-rwxr-xr-x  3 hwennborg  wheel  134708940 Oct 11 16:23 out/release.344190/Google Chrome Framework.framework/Versions/A/Google Chrome Framework

Diff: 2.1 MB, so looking good.

Is the warning still a problem? I'm guessing nothing has changed since before the dev meeting last week.

The warning is still firing (see e.g. https://ci.chromium.org/buildbot/chromium.clang/ToTMac/2268)

I'm not sure it's a problem though. If I understand protected visibility correctly, the warning sounds like this is working as intended.

The way to get rid of the warning would be to not build swiftshader with protected visibility, which I think is an unusual choice for shared libraries these days.

The new Clang roll landed, and the size dropped again: https://chromeperf.appspot.com/report?sid=feff42214b11abb6a7afbb8573d8510253e20ce15fd610ddcf28529b78383b87&start_rev=586424&end_rev=591945

tapted, simonhatch: Are we confident this is getting monitored now, or should I file a separate bug to track that?