[ELF] Don't warn on undefined symbols if UnresolvedPolicy::Ignore is used

[MaskRay]

Summary

Add a condition UnresolvedPolicy::Ignore to elf::warnUnorderedSymbol to suppress Sym->isUndefined() warnings from both

1. --symbol-ordering-file=
2. .llvm.call-graph-profile

If --unresolved-symbols=ignore-all is used,

no "undefined symbol" error/warning is emitted. It makes sense to not warn unorderable symbols.

Otherwise,

If an executable is linked, the default policy UnresolvedPolicy::ErrorOrWarn will issue a "undefined symbol" error. The unorderable symbol warning is redundant.

If a shared object is linked, it is possible that only part of object files are used and some symbols are left undefined. The warning is not very necessary.
  In particular for .llvm.call-graph-profile, when linking a shared object, a call graph profile may contain undefined symbols. This case generated a warning before but it will be suppressed by this patch.

[ruiu]

You should skip an undefined symbol if it is created from a DSO, no? Looks like just skipping all undefined symbols there also suppresses legitimate warnings.

[MaskRay]

Use UnresolvedPolicy::Ignore

[MaskRay]

.

[ruiu]

Does that make sense? I still feel like ignoring undefined symbols spoils the point of calling that function in the first place.

[davidxl]

What is the intended use of such warnings?

[MaskRay]

In D53044#1259572, @ruiu wrote:

Does that make sense? I still feel like ignoring undefined symbols spoils the point of calling that function in the first place.

Do you think that when linking a shared object, users should specify --no-warn-symbol-ordering to suppress the unable to order undefined symbol: warning?

[MaskRay]

elf::warnUnorderableSymbol(const Symbol *Sym) is used in 1) reading call graph profile sections 2) --symbol-ordering-file=

FWIW, ld.bfd just ignores undefined symbols with --symbol-ordering-file=. I don't like that behavior and I think lld is right to give some warnings as it currently does. But regarding call graph profile, when linking a shared object with some undefined symbols, I feel the UnresolvedPolicy::Ignore mechanism (--shared implies Ignore) can be reused to suppress such warnings.

[ruiu]

I don't think I understand the point of this patch. You can't reorder undefined symbols, so why does CG-profile section contains undefined symbol in the first place?

[MaskRay]

In D53044#1259600, @ruiu wrote:

I don't think I understand the point of this patch. You can't reorder undefined symbols, so why does CG-profile section contains undefined symbol in the first place?

My understanding is that if you link an executable, all .cg_profile symbols should be defined. However, with -shared, it is possible that only partial object files are linked and some symbols are left undefined. By separating From and To symbols into different modules, the .cg_profile From, To, count directives are essentially rendered useless, but I feel this is intended and no warnings should be given.

[ruiu]

It needs to be explained. I couldn't get it just by reading two lines of code.

[MaskRay]

Add comment why we use

if (!Sym->isUndefined() ||
    Config->UnresolvedSymbols != UnresolvedPolicy::Ignore)

[grimar]

ELF/Driver.cpp

689

It reads strangely: "If a symbol is not undefined (or ...) then warnUnorderableSymbol"
Makes think that we reporting a warning for defined symbols.

I think if that check should be made it should be inside the warnUnorderableSymbol

Also, should it check the if (Config->Shared) condition at some point?

[MaskRay]

ELF/Driver.cpp

689

I think if that check should be made it should be inside the warnUnorderableSymbol

Then this option will also affect --symbol-ordering-file= (ld.bfd implements this option but gold doesn't. ld.bfd does not warn undefined symbols.). I think this also seems fine.

Config->Shared

Checking UnresolvedPolicy::Ignore looks good enough as this is also a symbol resolution process.

[MaskRay]

Lower UnresolvedPolicy check to warnUnorderableSymbol

[MaskRay]

.

[MaskRay]

After thinking it over, I decide to sink the UnresolvedPolicy::Ignore condition to elf::warnUnorderedSymbol to suppress Sym->isUndefined() warnings from both 1) --symbol-ordering-file= and 2) .llvm.call-graph-profile

The test symbol-ordering-file-warnings.s uses --unresolved-symbols=ignore-all to check warnings of Sym->isUndefined(). But I think it makes more sense to just don't warn the unorderable symbol case, as the error: undefined symbol: $symbol error/warning is also suppressed. If the symbol does not exist, users will still see warning: symbol ordering file: no such symbol: $symbol. This change does not make the status worse.

[MaskRay]

Reword the comment

[ruiu]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

Please do not change this -- this code was written in the way it is to reduce indentation depth.

[MaskRay]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

Should I use if (Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore) then?

I actually find dispatching by SymbolKind first makes the code a bit easier to understand:

• Undefined
• Shared
• Others (Defined)

For the Defined case, fine-grained conditions are checked for absolute/syunthetic/discarded.

I lean to this style but if you have strong opinion on this, I can change it.

[ruiu]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

Yeah, maybe you should write that if at the beginning of this function to exit early.

[MaskRay]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

But that way:

  if (Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore)
    Warn(": unable to order undefined symbol: ");
  else if (Sym->isShared())
    Warn(": unable to order undefined symbol: ");
  else if (Sym->isShared())
    Warn(": unable to order shared symbol: ");
////// Sym->isUndefined() is possible here
  else if (D && !D->Section)
    Warn(": unable to order absolute symbol: ");
  else if (D && isa<OutputSection>(D->Section))
    Warn(": unable to order synthetic symbol: ");
  else if (D && !D->Section->Repl->Live)
    Warn(": unable to order discarded symbol: ");

Will that cause a problem?

[MaskRay]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

Sorry I misread your words. I think you mean:

void elf::warnUnorderableSymbol(const Symbol *Sym) {
  if (!Config->WarnSymbolOrdering ||
      Config->UnresolvedSymbols == UnresolvedPolicy::Ignore)
    return;

But this way UnresolvedPolicy::Ignore also changes how Shared Defined symbols are interpreted. UnresolvedPolicy::Ignore is also used otherwhere, but only related to how undefined symbols are interpreted.

[ruiu]

ELF/Symbols.cpp

280–288 ↗

(On Diff #169072)

I mean you can exit early if Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore, can't you?

[MaskRay]

Use early exit

if (!Config->WarnSymbolOrdering ||
    (Sym->isUndefined() &&
     Config->UnresolvedSymbols == UnresolvedPolicy::Ignore))

[ruiu]

ELF/Symbols.cpp

268–271 ↗

(On Diff #169114)

It's a nit, but I wouldn't merge the two conditions to make it look nicer.

if (!Config->WarnSymbolOrdering)
  return;

// comment
if (Sym->isUndefined() && ...)
  return;

The first condition is obvious, and it is better not to mix it with some more complicated condition, which needs a comment.

[MaskRay]

Use ruiu's preferred early-exit style

[MaskRay]

.

[ruiu]

ELF/Symbols.cpp

284 ↗

(On Diff #169116)

This is not related.

[MaskRay]

ELF/Symbols.cpp

284 ↗

(On Diff #169116)

I just made a change while you were commenting..

[ruiu]

LGTM

ELF/Driver.cpp

for (StringRef L : args::getLines(MB)) {
     if (const InputSectionBase *FromSB = FindSection(Fields[0]))
       if (const InputSectionBase *ToSB = FindSection(Fields[1]))
         Config->CallGraphProfile[std::make_pair(FromSB, ToSB)] += Count;
   }
 }
 
 template <class ELFT> static void readCallGraphsFromObjectFiles() {
   auto FindSection = [&](const Symbol *Sym) -> const InputSectionBase * {
+    if (!Sym->isUndefined())
       warnUnorderableSymbol(Sym);
     if (const auto *SymD = dyn_cast<Defined>(Sym))
       return dyn_cast_or_null<InputSectionBase>(SymD->Section);
     return nullptr;
   };
 
   for (auto File : ObjectFiles) {
     auto *Obj = cast<ObjFile<ELFT>>(File);
     for (const Elf_CGProfile_Impl<ELFT> &CGPE : Obj->CGProfile) {
       const InputSectionBase *FromSB =
           FindSection(&Obj->getSymbol(CGPE.cgp_from));
       const InputSectionBase *ToSB = FindSection(&Obj->getSymbol(CGPE.cgp_to));
       if (!FromSB || !ToSB)

test/ELF/cgprofile-shared-warn.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld -shared %t.o -o /dev/null 2>&1 | count 0
+    .section    .text.A,"ax",@progbits
+    .globl  A
+A:
+    callq B
+
+    .cg_profile A, B, 10