Issue 814412  has been merged into this issue.

Another approach: is it possible to look at a chrome binary and determine if ICF is working?

what is the other approach?

Hi Rahul, can we rely on the ratio of functions that share the same addresses? For example,

$ nm chrome.debug | grep -o "^[0-9a-f]+[ ]+[rRtT] " | sort -u | wc -l
391777

$ nm chrome.debug | grep -o "^[0-9a-f]+[ ]+[rRtT] " | wc -l
558027

There are ~30% redundant symbols. Assuming that ICF folds a significant amount of functions, a 20% threshold should be OK to discriminate icf and no-icf?

Isn't it enough to check the command line to see if --icf=all was passed?
Can we get the linker command line from the binary?

The way I see it, we're not really trying to test how much savings ICF gives us.
We only need to verify that we have it enabled.

#3: The other approach is porting small unittests into our nightly tests.

#5: Unfortunately, I cannot find -Wl,--icf=all in the debug info, nor the linker command line producing chrome. It might deserve a bug.

On the other hand, test how much savings ICF gives us also ensures that we have it enabled. If this can be done, we won't need the compiler flag check.

Just built a chrome without icf. The code base is different than that in #4 but it shouldn't matter:

$ nm chrome | egrep -o "^[0-9a-f]+[ ]+[rRtT] " | sort -u | wc -l
525478

$ nm chrome | egrep -o "^[0-9a-f]+[ ]+[rRtT] " | wc -l
567629

The redundancy is ~7.4%.

Hi Rahul, I saw that you have counts of "sections merged" in the other bug. Is it easily available? E.g., a linker flag to show to info. Or is it requires debug or even experimental builds of linker?

https://bugs.chromium.org/p/chromium/issues/detail?id=641042

Aha.. I think I used the '--print-icf-sections' flag:

    --print-icf-sections        List folded identical sections on stderr

and passed the stderr to 'wc -l' to get a count.
We could use this somehow.

 Issue 814434  has been merged into this issue.

'--print-icf-sections' is a linker flag. Not sure if having the test in BuildPackages is desirable, although logging it and check later is possible (similar to what static analyzer is going to do).

Would checking by nm be easier?

I just sent a CL for review that tests for this in the chromeos-chrome ebuild using the technique in comment#4: https://chromium-review.googlesource.com/1025133

Here are the numbers from my testing of chrome builds with and without ICF, for chell (amd64) and elm (arm) boards:

chell:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :  533,239
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :  362,219

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :  540,886
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :  498,762


elm:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           : 1,167,388
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l : 1,001,091

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           : 1,327,803
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l : 1,285,385


The number of duplicate text addresses in nm output is ~170,000 for both amd64 and arm when ICF is enabled.
With ICF disabled, the number of duplicate text addresses is ~42,000 for both amd64 and arm.

The change in the CL checks for a threshold of 100,000 duplicate text addresses, and has plenty of safety margin on either side.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/b694ee00b3c0aff0130dfc768bb096423deb3269

commit b694ee00b3c0aff0130dfc768bb096423deb3269
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Tue Apr 24 07:30:11 2018

chromeos-base/chromeos-chrome: check that chrome was built with ICF enabled.

For both amd64 and arm, when ICF (Identical Code Folding) is enabled,
there is ~170,000 drop in the number of unique addresses of text symbols.
We check here that the drop is at least 100,000.

BUG= chromium:813272 
TEST='emerge-chell chromeos-base/chromeos-chrome' works.
TEST='emerge-elm chromeos-base/chromeos-chrome' works.

Change-Id: I3213135b48525063fc922f98244113127b6b3b0b
Reviewed-on: https://chromium-review.googlesource.com/1025133
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Ting-Yuan Huang <laszio@chromium.org>

[modify] https://crrev.com/b694ee00b3c0aff0130dfc768bb096423deb3269/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/3f43effcfdb77fd62f098b2546a1ff6af50f43c6

commit 3f43effcfdb77fd62f098b2546a1ff6af50f43c6
Author: Toni Barzic <tbarzic@chromium.org>
Date: Wed Apr 25 08:18:55 2018

Revert "chromeos-base/chromeos-chrome: check that chrome was built with ICF enabled."

This reverts commit b694ee00b3c0aff0130dfc768bb096423deb3269.

Reason for revert:
Breaking terra-chrome-pfq. See https://bugs.chromium.org/p/chromium/issues/detail?id=836296

Original change's description:
> chromeos-base/chromeos-chrome: check that chrome was built with ICF enabled.
> 
> For both amd64 and arm, when ICF (Identical Code Folding) is enabled,
> there is ~170,000 drop in the number of unique addresses of text symbols.
> We check here that the drop is at least 100,000.
> 
> BUG= chromium:813272 
> TEST='emerge-chell chromeos-base/chromeos-chrome' works.
> TEST='emerge-elm chromeos-base/chromeos-chrome' works.
> 
> Change-Id: I3213135b48525063fc922f98244113127b6b3b0b
> Reviewed-on: https://chromium-review.googlesource.com/1025133
> Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
> Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
> Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
> Reviewed-by: Ting-Yuan Huang <laszio@chromium.org>

Bug:  chromium:813272 ,  chromium:836296 
Change-Id: Ied7022aafa20753ce9d97f310db82bf95ade84f8
Reviewed-on: https://chromium-review.googlesource.com/1026590
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Toni Barzic <tbarzic@chromium.org>

[modify] https://crrev.com/3f43effcfdb77fd62f098b2546a1ff6af50f43c6/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

The numbers in comment#14 were from a local build without thinlto.
It turns out that the number of symbols is very different when thinlto is enabled for chrome.
This resulted in waterfall build failures in *-chrome-pfq builders and the earlier CL had to be reverted.

Here are the numbers from chrome builds with and without thinlto, and with and without ICF:

chell without thinlto:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :   533,239
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :   362,219
    Number of duplicate symbols                                  :   171,020

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :   540,886
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :   498,762
    Number of duplicate symbols                                  :    42,124


elm without thinlto:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           : 1,167,388
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l : 1,001,091
    Number of duplicate symbols                                  :   166,297

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           : 1,327,803
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l : 1,285,385
    Number of duplicate symbols                                  :    42,418


chell with USE=thinlto:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :   350,916
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :   284,228
    Number of duplicate symbols                                  :    66,688

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :   392,643
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :   390,311
    Number of duplicate symbols                                  :     2,332

elm with USE=thinlto:
  icf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           :   899,748
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l :   836,181
    Number of duplicate symbols                                  :    63,567

  noicf:
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | wc -l           : 1,062,000
    nm chrome | egrep -o "^[0-9a-f]+[ ]+[tT] " | sort -u | wc -l : 1,059,417
    Number of duplicate symbols                                  :     2,583


Without thinlto, ICF was resulting in ~170,000 duplicate symbols, and the CL was testing that the number is at least 100,000.

With thinlto enabled, ICF is resulting in ~65,000 duplicate symbols.
With thinlto enabled and ICF disabled, the number of duplicate symbols is ~2,500.

Based on these numbers, a test that the number of duplicate symbols is at least 10,000 makes sense.
This test will catch the case where thinlto is enabled, but ICF is disabled.
It'll not trigger for any other combination of these two features.
If we have another test to check that thinlto is enabled, we should be fine.

You are saying that you are willing to blow up the build by taken out of thin air numbers? If you want to run tests like that, make sure they happen asynchronously to the build. You must not put this into the ebuild.

First off, there is precedent for doing these kind of tests in the chrome ebuild: https://chromium-review.googlesource.com/333879

Secondly, for this approach of counting duplicate symbols to work, nm has to be run against the unstripped chrome binary, which is easily available during the emerge. After the ebuild is done, I don't know of an easy way to access the original unstripped chrome binary. If you're aware of a way to do that, please let me know.

And lastly, the earlier CL went through the CQ. Are we not testing chrome ebuild changes in the CQ? Are chrome ebuild changes supposed to go through some other continuous-integration system?

If a change to an ebuild goes through CQ, then blows up the waterfall builders, and a revert of that change doesn't fix the build breakage, and all this is expected behavior, I'd say the overall design of our build and continuous integration systems is broken to begin with.

1) This change made fatal problem that was hard to read easier to debug, but still fatal. What you are doing is using an optional issue and make it fatal.

2) You *could* signal the failure in non-fatal ways and process it out of band. There are many ways to do it. Send yourself an email from the builder, leave breadcrumbs in the logs and process them out of band to find failures etc. But the build loop should ideally be independent of the testing loop. That ChromeOS likes to mix both is pretty messed up IMHO.

3) Of course the CQ does *not* test ebuild changes, because the existence of the Chrome PFQ prevents testing ebuild changes.

And why? Of course ChromeOS build system is broken. The original reason why it is broken is because it is slow to start with. If it was fast we would not need things like PFQs. Can the toolchain team help with making the build faster?

ihf: I don't think build time has anything to do with this bug. 
We constantly pay attention to build time and we have improved it several times.
To "fix" the builds and make them much faster we should probably be using a different programming language or machines that are at least 10X faster. 
Please keep this bug to the point. 

The point is you are adding a random number generator and calling it a test. Fix this.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/b0c2d48d1723aaa2bf1f6582500b91ffce252b34

commit b0c2d48d1723aaa2bf1f6582500b91ffce252b34
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Sat May 12 21:25:22 2018

debug_info_test: check that chrome was built with ICF enabled

For both amd64 and arm, when ICF (Identical Code Folding) is enabled,
there is >60,000 drop in the number of unique addresses of text symbols.
We check here that the drop is at least 10,000.

See  https://crbug.com/813272#c18  for details.

BUG= chromium:813272 
TEST='./debug_info_test.py /build/chell/usr/lib/debug/opt/google/chrome/chrome.debug'
TEST='./debug_info_test.py /build/elm/usr/lib/debug/opt/google/chrome/chrome.debug'

Change-Id: I23d1b48e6547bf3042096bed1c566ec50c5b2d66
Reviewed-on: https://chromium-review.googlesource.com/1053388
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Yunlian Jiang <yunlian@chromium.org>

[modify] https://crrev.com/b0c2d48d1723aaa2bf1f6582500b91ffce252b34/debug_info_test/debug_info_test.py
[add] https://crrev.com/b0c2d48d1723aaa2bf1f6582500b91ffce252b34/debug_info_test/check_icf.py