[PATCH] D43234: [ELF] - Fix case of using both --icf and --symbol-ordering-file together.

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 15 07:59:32 PST 2018


jhenderson added a comment.

In https://reviews.llvm.org/D43234#1007032, @ruiu wrote:

> I think that you create a symbol ordering file from profiler output. COMDAT names that the profiler uses is the one that is selected by ICF, no? So, the generated symbol ordering file naturally uses the COMDAT names that were selected by ICF. So I don't think we need to do anything about it.


I think this is the normal use case, but I don't think it's the only use case, so I'm not sure we can assume that these files have always been auto-generated. I think that warning for the removed symbol is probably sufficient though, to alert users to something going wrong. This is the current behaviour, following my earlier change, and see also https://reviews.llvm.org/D43336, where I realised that it should be tested explicitly. One counter-argument to this is that profilers may not load the symbols in symbol table order, so this means that they could load the "discarded" symbol first (since it appears in the symbol table), and try to add it to the order file, instead of the selected one, resulting in warnings for what should seem like a perfectly good input on first look.

I think the only way we can avoid warnings in both cases is to disable --icf with --symbol-ordering-file, at least for the contents of that file (symbols in the ordering file could still be in "selected" sections, but not removed ones). I'm not sure if this is ideal either though.


https://reviews.llvm.org/D43234





More information about the llvm-commits mailing list