[PATCH] D44180: Improve --warn-symbol-ordering.

[Rafael Avila de Espindola via llvm-commits]

For giving a user as much information as possible we would warn only
when an ICFed section causes a symbol to not be at the desired location.

But note that we don't even warn when two symbols are in the same
section:

.text
.global foo
foo:
.global bar
bar:

The symbols foo and bar will always have the same values, regardless of
where they are in the order file.

I am OK with warning on both cases, but it feels like that would be a
separate warning. A non existing symbol showing up in the order file
probably means there is something wrong with how that files was
created. Two symbols that can't be independently moved because they are
in the same section (because of ICF or not) seems like a normal event in
most cases.

Cheers,
Rafael

James Henderson via Phabricator via llvm-commits
<llvm-commits at lists.llvm.org> writes:

> jhenderson added a comment.
>
> I'm not sure about the ICF behaviour change. If a user has two functions, and wants them to appear in two very different locations in the order file for some reason, ICF will silently break this order. Example order file:
>
>   identical1
>   not_identical
>   identical2
>
> With ICF enabled, identical1 and identical2 will be placed at the same location in the output (as is normal for ICF), and so there will not be one at the start and one at the end as expected. Therefore, we haven't created the order that was explicitly asked for. I think therefore it is reasonable to warn still in this case.
>
> The flip-side is that if only one of identical1 or identical2 is specified in the order file, then we should probably not warn, since it is possible to order the symbols in this case! One possible solution is to record all symbols in the order file in sections which have been discarded due to ICF, then after completing the loop, looking for any such symbols whose section doesn't have the Priority that would be expected. It's not an ideal solution though.
>
>
>
> ================
> Comment at: lld/ELF/Writer.cpp:1048
>  
> -      auto *D = dyn_cast<Defined>(Sym);
> -      if (Config->WarnSymbolOrdering) {
> -        if (Sym->isUndefined())
> -          warn(File->getName() +
> -               ": unable to order undefined symbol: " + Sym->getName());
> -        else if (Sym->isShared())
> -          warn(File->getName() +
> -               ": unable to order shared symbol: " + Sym->getName());
> -        else if (D && !D->Section)
> -          warn(File->getName() +
> -               ": unable to order absolute symbol: " + Sym->getName());
> -        else if (D && !D->Section->Live)
> -          warn(File->getName() +
> -               ": unable to order discarded symbol: " + Sym->getName());
> +      auto Warn = [&](StringRef Msg) {
> +        if (Config->WarnSymbolOrdering)
> ----------------
> We should probably avoid using "Warn" for this name, as it is too easy for a developer to accidentally call the wrong function (e.g. via type), and bypass the check. Perhaps "WarnOrder" or similar.
>
>
> https://reviews.llvm.org/D44180
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits