[lld-macho] Prevent assertions for aliases to weak_def_can_be_hidden symbols

[paulkirth]

Summary

In https://reviews.llvm.org/D137982 we found that on Mach-O private
aliases could trigger an assert in lld when the aliasee was a
weak_def_can_be_hidden symbol.

This appears to be incorrect, and should be allowed in Mach-O.
Disallowing this behavior is also inconsistent with how ld64 handles
a private alias to weak_def_can_be_hidden symbols.

This patch removes the assert and tests that LLD handles such aliases
gracefully.

[paulkirth]

lld/test/MachO/weak-def-can-be-hidden.s

142

oops, will remove whitespace changes.

[paulkirth]

Remove newlines in test file

[int3]

Hm interesting. I'm actually wondering if we are handling these aliases correctly in general. I wasn't aware that there were section-based aliases; alias-symbols.s only tests the non-section-based types.

[paulkirth]

In D141082#4032085, @int3 wrote:

Hm interesting. I'm actually wondering if we are handling these aliases correctly in general.

That's actually one of the reasons I made the patch. I'm not the most familiar w/ Mach-O semantics, but I believe its perfectly fine to have an alias to a weak or weak_def_can_be_hidden symbol. I wanted to double check with the experts that 1) that assumption is correct, and 2) if there is broader problem than just a missing case in an assertion.

I wasn't aware that there were section-based aliases; alias-symbols.s only tests the non-section-based types.

hmm, you know I just did the simple thing and added the test here since it used weak_def_can_be_hidden already. Do you think we should move this test into alias.s or maybe somewhere else?

[int3]

Okay so I looked into this a bit more and I don't think there are actually section-based aliases. Creating an alias to a Defined symbol (i.e. defined in the current object file) causes llvm-mc to emit a regular non-aliasing section symbol. Your sym.n_type & N_INDR check is not doing what you think it is -- n_type is not a set of disjoint flags, but a bunch of overlapping values. sym.n_type & N_INDR happens to be true for n_type == N_SECT, but that doesn't mean n_type is an alias.

So I think we should just delete the assertion, and keep the test as-is (with an update to the comment)

lld/test/MachO/weak-def-can-be-hidden.s

119

maybe something like "an alias is the only way to get llvm-mc to set .weak_def_can_be_hidden on an already-hidden symbol. Verify that LLD can handle these double-hidden symbols gracefully"

[int3]

(I verified the above by looking at the output of llvm-readobj --syms)

[paulkirth]

In D141082#4032313, @int3 wrote:

Your sym.n_type & N_INDR check is not doing what you think it is -- n_type is not a set of disjoint flags, but a bunch of overlapping values. sym.n_type & N_INDR happens to be true for n_type == N_SECT, but that doesn't mean n_type is an alias.

Thanks for pointing out the subtlety here. I was definitely under the wrong impression.

So I think we should just delete the assertion, and keep the test as-is (with an update to the comment)

Sounds good. I'll update the patch with those suggestions.

[paulkirth]

Address comments

• remove assert
• update comment in test file

[int3]

Thank you!

lld/test/MachO/weak-def-can-be-hidden.s

120

lld/MachO/InputFiles.cpp

if (sym.n_type & N_EXT) {
     else if (isWeakDefCanBeHidden)
       isPrivateExtern = true;
     return symtab->addDefined(
         name, isec->getFile(), isec, value, size, sym.n_desc & N_WEAK_DEF,
         isPrivateExtern, sym.n_desc & N_ARM_THUMB_DEF,
         sym.n_desc & REFERENCED_DYNAMICALLY, sym.n_desc & N_NO_DEAD_STRIP,
         isWeakDefCanBeHidden);
   }
-  assert(!isWeakDefCanBeHidden &&
-         "weak_def_can_be_hidden on already-hidden symbol?");
   bool includeInSymtab = !isPrivateLabel(name) && !isEhFrameSection(isec);
   return make<Defined>(
       name, isec->getFile(), isec, value, size, sym.n_desc & N_WEAK_DEF,
       /*isExternal=*/false, /*isPrivateExtern=*/false, includeInSymtab,
       sym.n_desc & N_ARM_THUMB_DEF, sym.n_desc & REFERENCED_DYNAMICALLY,
       sym.n_desc & N_NO_DEAD_STRIP);
 }
 

lld/test/MachO/weak-def-can-be-hidden.s

_foo:
   retq
 
 #--- weak-autohide-foo.s
 .globl _foo
 .weak_def_can_be_hidden _foo
 _foo:
   retq
 
+# An alias is the only way to set .weak_def_can_be_hidden on an already-hidden symbol.
+# Veryify that LLD can handle these double-hidden symbols gracefully.
+.set l_foo, _foo
+
 #--- weak-foo-pe.s
 .private_extern _foo
 .globl _foo
 .weak_definition _foo
 _foo:
   retq
 
 #--- weak-autohide-foo-pe.s
 .private_extern _foo
 .globl _foo
 .weak_def_can_be_hidden _foo
 _foo:
   retq
 
 #--- ref-foo.s
 .globl _bar
 _bar:
   callq _foo
   retq