<div dir="ltr">Hi James,<div><br></div><div>Thanks for the feedback.<br></div><div><div><br></div><div>1. There exists some hacking in chromium code base to make functions distinct. That's why chromium developers have requested for this feature in compiler to avoid this sort of hacking. <br></div><div>2. I found that there are cases unwanted symbols show up in the addrsig table. I'm looking at this problem. </div><div>3. This sounds like a better approach to me. We can do it like using a new attribute to annotate functions that should show up in addrsig table and use safe icf mode. </div></div><div><br></div><div>I'll try investigating unwanted symbols in the addrsig table before the last approach.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 7, 2021 at 12:26 PM James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto">Three things:</div><div dir="auto"><br></div><div dir="auto">1. if chrome already has a mechanism/hack to make functions distinct in the face of icf=all, do we really need to add a compiler feature?</div><div dir="auto"><br></div><div dir="auto">2. Are we sure functions aren't ending up in the addrsig table spuriously (and that the addrsig table isn't getting lost)? IOW: are the additional functions that get emitted with icf=safe really needed (as far as reasonable compiler analysis can determine), or should/could they have been removed?</div><div dir="auto"><br></div><div dir="auto">3. If we _do_ need to implement something here (which I'm skeptical of) it would be better to add a compiler flag which adjusts how the contents of the addrsig table are computed, vs adding a new linker mechanism.</div><div dir="auto"><br></div><div dir="auto">You'd continue to link with icf=safe, but the compiler option would allow more functions to be merged, by putting fewer into addrsig.</div><div dir="auto"><br></div><div dir="auto">This would also allow the "unsafe" semantics to be per-TU instead of global for the entire link, allowing you not to break other libraries that may be linked into the same binary.</div><div dir="auto"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 7, 2021, 1:32 PM Reid Kleckner <<a href="mailto:rnk@google.com" rel="noreferrer" target="_blank">rnk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Thanks for gathering the data, Zequan. For more context, I'll add that chrome.dll is around 163MB, so saving 6MB is around 3.5% of binary size. I feel like this is a compelling use case for aggressive ICF.<div><br></div><div>Chrome has historically used MSVC ICF, which is aggressive, and it can make debugging challenging. We're looking for a way that we can selectively power down ICF for functions that developers are interested in. I think it would be reasonable to have a compiler flag that emits an extra section with the same format as llvm_addrsig, with the semantics that it blocks ICF even in the aggressive mode.</div><div><br></div><div>For the user interface, we can either invent a new attribute, (noicf, blockicf, disable_icf), document that it only works with linkers that support the new section (LLD), and go that way. Alternatively we can overload an existing attribute like __attribute__((used)). I think I favor the new attribute. It makes it easier to search and find the relevant documentation.</div><div><br></div><div>Does that seem reasonable?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 7, 2021 at 10:06 AM Zequan Wu <<a href="mailto:zequanwu@google.com" rel="noreferrer noreferrer" target="_blank">zequanwu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I got some data for the size increase from safe icf to all icf. The 14 MB I initially found was due to .addrsig not emitted on COFF thinlto.<div>After enabling it, the size of chrome.dll is increased by 6 MB when switching from all icf to safe icf on windows and 5 MB increase on linux. </div><div><br></div><div>Here is a breakdown of chrome.dll section size increase on Windows.<br>5574656 bytes increase in .text<br>643072 bytes increase in .rdata<br>573440 bytes increase in .pdata<br>57344 bytes increase in .reloc<br>407152 bytes increase in rvatable<br></div><div><br></div><div>I dumped the sum of section size removed and selected/removed sections count by icf in ELF and COFF as below. </div><div><div>all-icf.win.txt:<br>total size removed: 10763454 selected: 19712 removed: 153544</div>safe-icf.win.txt:<br>total size removed: 6122783 selected: 8653 removed: 48999<br>all-icf.linux.txt:<br>total size removed: 10423699 selected: 22254 removed: 158806<br>safe-icf.linux.txt:<br>total size removed: 6165470 selected: 9313 removed: 53981</div><div><br></div><div>There is not much difference. So, safe icf is performing the same on linux and windows. I think we have enough motivation to add a new section for disabling icf.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 25, 2021 at 10:57 AM Reid Kleckner <<a href="mailto:rnk@google.com" rel="noreferrer noreferrer" target="_blank">rnk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I should add, though, that I agree with the final direction here: if the cost of safe ICF is low, it is much better to just use it instead of adding a whole new feature requiring documentation, etc. We need new measurements to show if the feature is justified.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 25, 2021 at 10:55 AM Reid Kleckner <<a href="mailto:rnk@google.com" rel="noreferrer noreferrer" target="_blank">rnk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">This is a bit of a hot take, but I'd rather make aggressive ICF (fold any identical code) the norm and scrap the C++ language guarantee about function pointer identity. I don't participate in C++ standardization, so my opinion doesn't carry much weight, but I lean towards giving the optimizer a free hand here. There are a number of other ways in which function pointer identity breaks down. Consider -fvisibility-inlines=hidden, which if we are being real, is the only way to compile big C++ projects, even those with explicit visibility annotations. Function pointer identity is just fragile and easy to break, and I don't encourage people to rely on it. If you need some kind of globally unique identifier, it's pretty straightforward to use data instead of code.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 23, 2021 at 2:55 PM James Y Knight via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">In general, I don't like the "icf=all" mode. It's not nice to have the linker break semantics of the program, and I think we should not encourage its use. (And, in particular, adding new features just for it seems like a bad idea.) Instead, we should make sure that icf=safe works as well as it possibly can, and encourage its use.<div><br></div><div>It's not possible to conclude anything based only on the size results, but it does indicate that some further investigation may be a good idea, to look into <i>why</i> icf=safe is apparently less-effective on the windows binary vs the linux one.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 23, 2021 at 2:50 PM Fangrui Song via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
On 2021-03-23, Zequan Wu wrote:<br>
>The size increase of chrome on Linux by switching from all ICF to safe ICF<br>
>is small.<br>
>All ICF:<br>
> text data bss dec hex filename<br>
>169314343 8472660 2368965 180155968 abcf640 chrome<br>
>Safe ICF:<br>
> text data bss dec hex filename<br>
>174521550 8497604 2368965 185388119 b0ccc57 chrome<br>
><br>
>On Windows, chrome.dll increases size by around 14 MB (12MB increases in<br>
>.text section).<br>
>All ICF:<br>
>Size of out\Default\chrome.dll is 170.715648 MB<br>
> name: mem size , disk size<br>
> .text: 141.701417 MB<br>
> .rdata: 22.458476 MB<br>
> .data: 3.093948 MB, 0.523264 MB<br>
> .pdata: 4.412364 MB<br>
> .00cfg: 0.000040 MB<br>
> .gehcont: 0.000132 MB<br>
> .retplne: 0.000108 MB<br>
> .rodata: 0.004544 MB<br>
> .tls: 0.000561 MB<br>
> CPADinfo: 0.000056 MB<br>
> _RDATA: 0.000244 MB<br>
> .rsrc: 0.285232 MB<br>
> .reloc: 1.324196 MB<br>
>Safe ICF:<br>
>Size of out\icf-safe\chrome.dll is 184.499712 MB<br>
> name: mem size , disk size<br>
> .text: 153.809529 MB<br>
> .rdata: 23.123628 MB<br>
> .data: 3.093948 MB, 0.523264 MB<br>
> .pdata: 5.367396 MB<br>
> .00cfg: 0.000040 MB<br>
> .gehcont: 0.000132 MB<br>
> .retplne: 0.000108 MB<br>
> .rodata: 0.004544 MB<br>
> .tls: 0.000561 MB<br>
> CPADinfo: 0.000056 MB<br>
> _RDATA: 0.000244 MB<br>
> .rsrc: 0.285232 MB<br>
> .reloc: 1.379364 MB<br>
><br>
>If an attribute is used and it affects unnamed_addr of a symbol, it<br>
>determines whether the symbols should show up in the .addrsig table.<br>
>All-ICF mode in ld.lld and lld-link ignore symbols in the .addrsig table,<br>
>if they belong to code sections. So, it won't have an effect on disabling<br>
>ICF.<br>
<br>
Is there something which can be improved on lld-link? Note also that<br>
the size difference between gold --icf={safe,all} is smaller than the<br>
difference between ld.lld --icf={safe,all}. Some may be due to gold<br>
performing "unsafe" safe ICF, some may suggest places where clang fails<br>
to add {,local_}unnamed_addr.<br>
<br>
<br>
>On Mon, Mar 22, 2021 at 10:19 PM Fangrui Song <<a href="mailto:maskray@google.com" rel="noreferrer noreferrer" target="_blank">maskray@google.com</a>> wrote:<br>
><br>
>><br>
>> On 2021-03-22, David Blaikie via llvm-dev wrote:<br>
>> >ICF: Identical Code Folding<br>
>> ><br>
>> >Linker deduplicates functions by collapsing any identical functions<br>
>> >together - with icf=safe, the linker looks at a .addressing section in the<br>
>> >object file and any functions listed in that section are not treated as<br>
>> >collapsible (eg: because they need to meet C++'s "distinct functions have<br>
>> >distinct addresses" guarantee)<br>
>><br>
>> The name originated from MSVC link.exe where icf stands for "identical<br>
>> COMDAT folding".<br>
>> gold named it "identical code folding" - which makes some sense because<br>
>> gold does not fold readonly data.<br>
>><br>
>> In LLD, the name is not accurate for two reasons: (1) the feature can<br>
>> apply to readonly data as well; (2) the folding is by section, not by<br>
>> function.<br>
>><br>
>> We define identical sections as they have identical content and their<br>
>> outgoing relocation sets cannot be distinguished: they need to have the<br>
>> same number of relocations, with the same relative locations, with the<br>
>> referenced symbols indistinguishable.<br>
>><br>
>> Then, ld.lld --icf={safe,all} works like this:<br>
>><br>
>> For a set of identical sections, the linker picks one representative and<br>
>> drops the rest, then redirects references to the representative.<br>
>><br>
>> Note: this can confuse debuggers/symbolizers/profilers easily.<br>
>><br>
>> lld-link /opt:icf is different from ld.lld --icf but I haven't looked<br>
>> into it closely.<br>
>><br>
>><br>
>> I find that the feature's saving is small given its downside<br>
>> (also increaded link time: the current LLD's implementation is inferior:<br>
>> it performs a quadratic number of comparisons among an equality class):<br>
>><br>
>> This is the size differences for the 'lld' executable:<br>
>><br>
>> % size lld.{none,safe,all}<br>
>> text data bss dec hex filename<br>
>> 96821040 7210504 550810 104582354 63bccd2 lld.none<br>
>> 95217624 7167656 550810 102936090 622ae1a lld.safe<br>
>> 94038808 7167144 550810 101756762 610af5a lld.all<br>
>> % size gold.{none,safe,all}<br>
>> text data bss dec hex filename<br>
>> 96857302 7174792 550825 104582919 63bcf07 gold.none<br>
>> 94469390 7174792 550825 102195007 6175f3f gold.safe<br>
>> 94184430 7174792 550825 101910047 613061f gold.all<br>
>><br>
>> Note that the --icf=all result caps the potential saving of the proposed<br>
>> annotation.<br>
>><br>
>> Actually with some large internal targets I get even smaller savings.<br>
>><br>
>><br>
>> ld.lld --icf=safe is safer than gold --icf=safe but probably misses some<br>
>> opportunities.<br>
>> It can be that clang codegen/optimizer fail to mark some cases as<br>
>> {,local_}unnamed_addr.<br>
>><br>
>> I know Chromium and the Windows world can be different:) But I'd still<br>
>> want to<br>
>> get some numbers first.<br>
>><br>
>><br>
>> Last, I have seen that Chromium has some code like<br>
>><br>
>> <a href="https://source.chromium.org/chromium/chromium/src/+/master:skia/ext/SkMemory_new_handler.cpp" rel="noreferrer noreferrer noreferrer" target="_blank">https://source.chromium.org/chromium/chromium/src/+/master:skia/ext/SkMemory_new_handler.cpp</a><br>
>><br>
>> void sk_abort_no_print() {<br>
>> // Linker's ICF feature may merge this function with other<br>
>> functions with<br>
>> // the same definition (e.g. any function whose sole job is to call<br>
>> abort())<br>
>> // and it may confuse the crash report processing system.<br>
>> // <a href="http://crbug.com/860850" rel="noreferrer noreferrer noreferrer" target="_blank">http://crbug.com/860850</a><br>
>> static int static_variable_to_make_this_function_unique = 0x736b;<br>
>> // "sk"<br>
>> base::debug::Alias(&static_variable_to_make_this_function_unique);<br>
>><br>
>> abort();<br>
>> }<br>
>><br>
>> If we want an approach to work with link.exe, I don't know what we can<br>
>> do...<br>
>> If no desire for link.exe compatibility, I can see that having a proper<br>
>> way marking the function<br>
>> can be useful... but in any case if an attribute is used, it probably<br>
>> should affect<br>
>> unnamed_addr directly instead of being called *icf*.<br>
>><br>
>><br>
>><br>
>> >On Mon, Mar 22, 2021 at 6:16 PM Philip Reames via llvm-dev <<br>
>> ><a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> ><br>
>> >> Can you define ICF please? And give a bit of context?<br>
>> >><br>
>> >> Philip<br>
>> >> On 3/22/21 5:27 PM, Zequan Wu via llvm-dev wrote:<br>
>> >><br>
>> >> Hi all,<br>
>> >><br>
>> >> Background:<br>
>> >> It's been a longstanding difficulty of debugging with ICF. Programmers<br>
>> >> don't have control over which sections should be folded by ICF, which<br>
>> >> sections shouldn't. The existing address significant table won't have<br>
>> >> effect for code sections during all ICF mode in both ld.lld and<br>
>> lld-link.<br>
>> >> By switching to safe ICF could mark code sections as unique, but at a<br>
>> cost<br>
>> >> of increasing binary size out of control. So, it would be good if<br>
>> >> programmers could selectively disable ICF in source code by annotating<br>
>> >> global functions/variables with an attribute to improve debugging<br>
>> >> experience and have the control on the binary size increase.<br>
>> >><br>
>> >> My plan is to add a new section table(`.no_icf`) to object files.<br>
>> Sections<br>
>> >> of all symbols inside the table should not be folded by all ICF mode.<br>
>> And<br>
>> >> symbols can only be added into the table by annotating global<br>
>> >> functions/variables with a new attribute(`no_icf`) in source code.<br>
>> >><br>
>> >> What do you think about this approach?<br>
>> >><br>
>> >> Thanks,<br>
>> >> Zequan<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> LLVM Developers mailing listllvm-dev@lists.llvm.orghttps://<br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> LLVM Developers mailing list<br>
>> >> <a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> >> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>> >><br>
>><br>
>> >_______________________________________________<br>
>> >LLVM Developers mailing list<br>
>> ><a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> ><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>><br>
>><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>