I think we should revert the roll if we're sure about this. There's no way that can be an intentional increase.

I'd like to know which sections grew before doing that. It could be something like extra debug info that's only present in test binaries and gets stripped from actual chrome binaries.

To be clear: you want to learn which sections grew, and *then* roll back, right? Because a 50GB jump in the build due to a compile change isn't so acceptable.

You should be able to figure out the increase by looking at the binaries linked to in the description, right?

I'm bumping this to P0 to make sure we deal with this quickly ...

If it's debug info or rodata, it could just be a configuration issue that we can fix in Chromium. If it's text, we should roll back. That would be a long investigation.

Can we roll back regardless while you do the investigation, since you should have data to look at? Is there something super-critical in this roll?

This is a debug build configuration. Are we really sure we care about clang emitting ginormous debug binaries, only on iOS? Assuming this isn't something obviously broken, this could be clang emitting more debug info, or doing something else that it will ultimately continue doing forever. We may just have to eat this build directory size increase. The text section actually looks like it got smaller between the two binaries linked above.

If we do care, let's do the revert. I don't have enough time today to debug this, and I'm not familiar enough with MachO to do it quickly.

Yes, I am really sure we care about a 50GB increase in our build size on our builders, regardless of the build configuration :). Disk space is limited on the macs and that amount of increase will also impact cycle time significantly.

I started a revert: https://chromium-review.googlesource.com/c/chromium/src/+/1170006

The revert landed in #582026

It was picked up here:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-simulator/36297
First base_unittests swarming job:
https://chromium-swarm.appspot.com/task?id=3f3ab6441c2c2810&refresh=10&show_raw=1
In the isolated inputs:
src/out/Debug-iphonesimulator/base_unittests.app/base_unittests	81095df4cc548ca1f1b66eb24e38842a95371a5f	493	135177824

Looking at the build before the revert:
https://chromium-swarm.appspot.com/task?id=3f3a816189820a10&refresh=10&show_raw=1
In the isolated inputs:
src/out/Debug-iphonesimulator/base_unittests.app/base_unittests	5550d8cf751dd4477e5bd63f7bdc1b99fc5d4191	493	202771984

That's a 64 MB drop, so it seems the revert helped.

dpranke, tikuta: Can you confirm that the bots are happy now?

I'll take a look to see if I can reproduce this locally.

#9,

Yes, disk usage of the bot seems reduced largely.
http://shortn/_hA1HXm3dMA

Bisecting over clang now. I'm very curious as to what we'll find :-)

Building locally...

gn args:

additional_target_cpus = [ "x86" ]
is_component_build = false
is_debug = true
symbol_level = 1
target_cpu = "x64"
target_os = "ios
clang_base_path="/work/llvm.combined/build.release" clang_use_chrome_plugins=false llvm_force_head_revision=true"


$ ninja -C build.release

$ gn clean out/release && ninja -C out/release base_unittests && ls -l out/release/base_unittests.app/base_unittests


clang 338452
-rwxr-xr-x  1 hwennborg  wheel  201572448 Aug 10 11:07 out/release/base_unittests.app/base_unittests

clang 337439
-rwxr-xr-x  1 hwennborg  wheel  134204368 Aug 10 11:29 out/release/base_unittests.app/base_unittests

> Yes, disk usage of the bot seems reduced largely.
> http://shortn/_hA1HXm3dMA

Thanks! Lowering the priority. We'll need to fix to roll clang again though.

Bisection points to r338122:

    [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY
    
    Summary:
    This commit introduces a new macro, _LIBCPP_HIDE_FROM_ABI, whose goal is to
    mark functions that shouldn't be part of libc++'s ABI. It marks the functions
    as being hidden for dylib visibility purposes, and as having internal linkage
    using Clang's __attribute__((internal_linkage)) when available, and
    __always_inline__ otherwise.
    
    It replaces _LIBCPP_INLINE_VISIBILITY, which was always using __always_inline__
    to achieve similar goals, but suffered from debuggability and code size problems.
    The full proposal, along with more background information, can be found here:
    
        http://lists.llvm.org/pipermail/cfe-dev/2018-July/058419.html
    
    This commit does not rename uses of _LIBCPP_INLINE_VISIBILITY to
    _LIBCPP_HIDE_FROM_ABI: this wide reaching but mechanical change can
    be done later when we've confirmed we're happy with the new macro.
    
    In the future, it would be nice if we could optionally allow dropping
    any internal_linkage or __always_inline__ attribute, which could result
    in code size improvements. However, this is currently impossible for
    reasons explained here: http://lists.llvm.org/pipermail/cfe-dev/2018-July/058450.html
    
    Reviewers: EricWF, dexonsmith, mclow.lists
    
    Subscribers: christof, dexonsmith, llvm-commits, mclow.lists
    
    Differential Revision: https://reviews.llvm.org/D49240


I'm ooo the rest of the day. If anyone wants to look into this in the meantime, please go ahead.

Thank you for bisecting that, Hans!

So, presumably the size blowup is from tons of copies of inline functions in STL classes like std::string. libc++ uses always_inline (and now internal_linkage) to hide these symbols and provide some extra ABI stability guarantees. However, the code size cost is high, and it's not clear to me that chrome needs this extra stability.

I think there are two reasonable solutions to this problem:
1. Find a flag to disable this internal_linkage / always_inline insanity, perhaps -D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN.
2. Enable ICF in the linker in debug builds.

Doing 1 requires knowing that iOS builds of Chrome do not statically link code compiled with other versions of libc++. I'm guessing that iOS Chrome is built from Apple-provided code, which is all in .dylibs pre-installed on iOS devices, and Chrome libraries compiled from source, even if they are in third_party. This would be the best outcome. We should confirm with the libc++ maintainers that this use case is supported.

Doing 2 may hurt debugability because stack traces can become confusing, and this is already in a debug build. I assume we already enable ICF in release builds, so there should be little to no code size change from this change there.

I left a comment on https://reviews.llvm.org/D49240 and it sounds like the change isn't supposed to blow up debug info size. So just providing more details there could help make progress.

Do we know the increase is mostly in debug info already? I made c#14 on the assumption that internal_linkage was actually increasing code size, but maybe that's completely wrong. It should actually be a net reduction in -O0 code size, so maybe you're right, it is debug info. But this is a symbol_level=1 build, why is there any debug info in the first place?

I was discussing this problem with somebody in the MK, and they suggested I use bloaty: https://github.com/google/bloaty

It basically tells us exactly what we want to know:

$ bloaty after_base_unittests -- before_base_unittests 
     VM SIZE                                 FILE SIZE
 --------------                           --------------
  +103% +51.4Mi String Table              +51.4Mi  +103%
  +105% +17.6Mi Symbol Table              +17.6Mi  +105%
   +49%  +827Ki Code Signature             +827Ki   +49%
   +92%  +263Ki Function Start Addresses   +263Ki   +92%
   +15% +4.50Ki __TEXT,__unwind_info      +4.50Ki   +15%
   +74% +3.52Ki Table of Non-instructions +3.52Ki   +74%
  -0.0%    -128 __TEXT,__const               -128  -0.0%
  -2.0%    -136 [__TEXT]                     -136  -2.0%
  [ = ]       0 [Unmapped]                   -480  -3.4%
  -1.0%    -516 __TEXT,__gcc_except_tab      -516  -1.0%
 -39.3% -1.07Ki [__LINKEDIT]                   +4   +33%
  -9.8% -5.61Mi __TEXT,__text             -5.61Mi  -9.8%
   +50% +64.5Mi TOTAL                     +64.5Mi   +50%

So, total size of __TEXT is down by 2%. This is good, removing always_inline has in fact gotten net code size savings.

I don't know what "code signature" is, but it's clear why function start addresses, symbol table, and string table metrics are all way, way up. We've introduced a ton of little helper functions from the STL.

So, it's not "debug info" in the sense that it's not DWARF, but the symbol table has grown dramatically.

I think what I said in c#14 could still be the solution, even if it misunderstood the problem. Most of the symbol table size increase is probably from inline functions that would normally be comdat folded, if it weren't for internal_linkage.

libc++ maintainer here (I made the commit that tripped you up). Answering a question asked above:

> Doing 1 requires knowing that iOS builds of Chrome do not statically link code compiled with other versions of libc++. I'm guessing that iOS Chrome is built from Apple-provided code, which is all in .dylibs pre-installed on iOS devices, and Chrome libraries compiled from source, even if they are in third_party. This would be the best outcome. We should confirm with the libc++ maintainers that this use case is supported.

Yes, the intent is for libc++ to support this use case. Ideally, we'd like to have a macro like `_LIBCPP_NO_INTERNAL_LINKAGE_FOR_ABI_UNSTABLE_SYMBOLS` that can be defined to disable the use of the `internal_linkage` attribute for functions that are not part of the ABI. This would allow ODR-merging and would reduce the number of symbols.

However, we haven't gotten around to doing it yet because we need to solve some issues with visibility. I (already) forgot the gory details of what the problem was, but you can read through this thread if you want all the details: http://lists.llvm.org/pipermail/cfe-dev/2018-July/058419.html.

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

commit d74b4a7338023093d2cc0ada1c8d9964e1f1faf8
Author: Hans Wennborg <hans@chromium.org>
Date: Tue Aug 14 17:08:19 2018

Re-land "Roll clang 337439:338452."

And configure libc++ to force inline ABI-hidden functions, as was its behaviour
before r338122. Without this, the symbol tables of Mach-O binaries grow
significantly due to including multiple definitions of these functions.

Bug:  866225 ,  870907 ,  872926 
Change-Id: I4382e9de57fc79b3baff3d329e338887cb4b133b
Reviewed-on: https://chromium-review.googlesource.com/1174546
Commit-Queue: Hans Wennborg <hans@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582951}
[modify] https://crrev.com/d74b4a7338023093d2cc0ada1c8d9964e1f1faf8/build/config/compiler/BUILD.gn
[modify] https://crrev.com/d74b4a7338023093d2cc0ada1c8d9964e1f1faf8/tools/clang/scripts/update.py

As a follow-up, maybe we can implement the changes that Louis suggested here:

> Actually, scratch that, it does work. One can either use `-D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` to restore the old behavior, or `-D_LIBCPP_HIDE_FROM_ABI=` to get odr linkage.

ODR linkage is probably best for Chromium. I had thought that earlier he implied we weren't able to do that.

Hans, do you mind doing some experiments with `-D_LIBCPP_HIDE_FROM_ABI=`? I don't have a local Mac, so I'd have to test it by running try jobs, downloading the artifacts, and inspecting manually.

I tried that first actually, but got lots of link errors. I guess it needs more support upstream before we can use it.

And reading https://reviews.llvm.org/D50652 it sounds like Louis wants to go back to always inline by default, and then work towards supporting the odr linkage, which I think suits us well.



FAILED: obj/base/x64/base_unittests 
TOOL_VERSION=1521192979 ../../build/toolchain/mac/linker_driver.py ../../../../llvm.combined/build.release/bin/clang++  -Xlinker -rpath -Xlinker @executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2 -Xlinker -sectcreate -Xlinker __TEXT -Xlinker __entitlements -Xlinker obj/base/base_unittests.xcent -arch x86_64 -Werror -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator11.2.sdk -stdlib=libc++ -mios-simulator-version-min=10.0 -Wl,-ObjC -o "obj/base/x64/base_unittests" -Wl,-filelist,"obj/base/x64/base_unittests.rsp"  -framework UIKit -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation
Undefined symbols for architecture x86_64:
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::find_first_of(char const*, unsigned long) const", referenced from:
      base::(anonymous namespace)::IsValidFeatureOrFieldTrialName(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in libbase.a(feature_list.o)
      logging::VlogInfo::VmodulePattern::VmodulePattern(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in libbase.a(vlog.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::get_allocator() const", referenced from:
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in bind_objc_block_unittest_arc.o
      std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > std::__1::operator+<char, std::__1::char_traits<char>, std::__1::allocator<char> >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in bind_objc_block_unittest_arc.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in scoped_nsobject_unittest_arc.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in at_exit_unittest.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in atomicops_unittest.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in barrier_closure_unittest.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::str() const in base64_unittest.o
      ...
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::find_last_not_of(char const*, unsigned long) const", referenced from:
      base::TextContentsEqual(base::FilePath const&, base::FilePath const&) in libbase.a(file_util.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::find_last_not_of(char, unsigned long) const", referenced from:
      base::Base64UrlEncode(base::BasicStringPiece<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > const&, base::Base64UrlEncodePolicy, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) in libbase.a(base64url.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::find_first_not_of(char const*, unsigned long) const", referenced from:
      base::debug::(anonymous namespace)::DemangleSymbols(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) in libbase.a(stack_trace_posix.o)
      base::FilePath::ReferencesParent() const in libbase.a(file_path.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::end() const", referenced from:
      decltype(fp.end()) std::__1::end<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in cpu_unittest.o
      base::StringTokenizerT<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__wrap_iter<char const*> >::StringTokenizerT(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in string_tokenizer_unittest.o
      testing::internal::XmlUnitTestResultPrinter::RemoveInvalidXmlCharacters(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in gtest.o
      std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > base::CollapseWhitespaceT<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool) in libbase.a(string_util.o)
      base::StringTokenizerT<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__wrap_iter<char const*> >::StringTokenizerT(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in libbase.a(command_line.o)
      base::(anonymous namespace)::AreAllSeparators(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in libbase.a(file_path.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::back() const", referenced from:
      base::FilePath::EndsWithSeparator() const in libbase.a(file_path.o)
  "std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::data() const", referenced from:
      std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> > std::__1::__pad_and_output<char, std::__1::char_traits<char> >(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> >, char const*, char const*, char const*, std::__1::ios_base&, char) in bind_objc_block_unittest_arc.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::seekoff(long long, std::__1::ios_base::seekdir, unsigned int) in bind_objc_block_unittest_arc.o
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char> >::overflow(int) in bind_objc_block_unittest_arc.o

Marking this fixed as we have a work-around, and trunk libc++ is also fixed.

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

commit 6c2527114e7363fe0656c4aaca8e580ae26b1a5b
Author: Reid Kleckner <rnk@google.com>
Date: Tue Aug 28 19:43:10 2018

Roll clang 338452:340740

Update clang_version from 7.0.0 to 8.0.0 and remove the code to remove
the old library directory.

Remove the _LIBCPP_HIDE_FROM_ABI define, it is no longer needed.

Bug:  871418 ,  872926 
Change-Id: Ic02dcfbf17c4190fc4359271d880b7e65bf7ba27
Reviewed-on: https://chromium-review.googlesource.com/1182196
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586816}
[modify] https://crrev.com/6c2527114e7363fe0656c4aaca8e580ae26b1a5b/build/config/compiler/BUILD.gn
[modify] https://crrev.com/6c2527114e7363fe0656c4aaca8e580ae26b1a5b/build/toolchain/toolchain.gni
[modify] https://crrev.com/6c2527114e7363fe0656c4aaca8e580ae26b1a5b/tools/clang/scripts/update.py

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

commit 7cb8c3d3ef8f1e0d24e7986de28f885d1e394242
Author: Reid Kleckner <rnk@chromium.org>
Date: Tue Aug 28 20:45:18 2018

Revert "Roll clang 338452:340740"

This reverts commit 6c2527114e7363fe0656c4aaca8e580ae26b1a5b.

Reason for revert: Breaks Google Chrome Linux x64 bot somehow:
https://ci.chromium.org/buildbot/chromium.chrome/Google%20Chrome%20Linux%20x64/35385

LINK ./chrome step fails with null pointer assertion in isa<ReturnInst>.

Original change's description:
> Roll clang 338452:340740
> 
> Update clang_version from 7.0.0 to 8.0.0 and remove the code to remove
> the old library directory.
> 
> Remove the _LIBCPP_HIDE_FROM_ABI define, it is no longer needed.
> 
> Bug:  871418 ,  872926 
> Change-Id: Ic02dcfbf17c4190fc4359271d880b7e65bf7ba27
> Reviewed-on: https://chromium-review.googlesource.com/1182196
> Commit-Queue: Reid Kleckner <rnk@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Hans Wennborg <hans@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#586816}

TBR=thakis@chromium.org,hans@chromium.org,rnk@chromium.org,dpranke@chromium.org,pcc@chromium.org

Change-Id: Ib32fe653d23cd7d4ce27d4cfc20faf2eec2a6ce0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  871418 ,  872926 
Reviewed-on: https://chromium-review.googlesource.com/1194830
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586839}
[modify] https://crrev.com/7cb8c3d3ef8f1e0d24e7986de28f885d1e394242/build/config/compiler/BUILD.gn
[modify] https://crrev.com/7cb8c3d3ef8f1e0d24e7986de28f885d1e394242/build/toolchain/toolchain.gni
[modify] https://crrev.com/7cb8c3d3ef8f1e0d24e7986de28f885d1e394242/tools/clang/scripts/update.py

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

commit d54494cbad8d5ff0a8cd74024902c8bd86258ae4
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Aug 30 15:59:01 2018

Roll clang 338452:340925

Update clang_version from 7.0.0 to 8.0.0 and remove the code to remove
the old library directory.

Remove the _LIBCPP_HIDE_FROM_ABI define, it is no longer needed.

R=hans@chromium.org,thakis@chromium.org

Bug:  871418 ,  872926 
Change-Id: Ibf6734500aa75a349fa772b225b8b15fddfe3661
Reviewed-on: https://chromium-review.googlesource.com/1194698
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587602}
[modify] https://crrev.com/d54494cbad8d5ff0a8cd74024902c8bd86258ae4/build/config/compiler/BUILD.gn
[modify] https://crrev.com/d54494cbad8d5ff0a8cd74024902c8bd86258ae4/build/toolchain/toolchain.gni
[modify] https://crrev.com/d54494cbad8d5ff0a8cd74024902c8bd86258ae4/tools/clang/scripts/update.py

libc++ is no longer using always_inline as of r345516, so c#2 is no longer a code size concern for switching on Windows.

Ach, wrong bug.