[PATCH] D133825: [lld-macho] Add support for N_INDR symbols

[Jez Ng via Phabricator via llvm-commits]

int3 marked an inline comment as done.
int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1216
+    if (auto *objFile = dyn_cast<ObjFile>(file)) {
+      for (const AliasSymbol *alias : objFile->aliases) {
+        if (const auto &aliased = symtab->find(alias->getAliasedName())) {
----------------
thakis wrote:
> This feels a bit inelegant. Conceptually, an alias and its target could be two map entries pointing at the same symbol – if it weren't for the fact that the alias can have more restrictive visibility :/
agreed heh


================
Comment at: lld/MachO/Driver.cpp:1216
+    if (auto *objFile = dyn_cast<ObjFile>(file)) {
+      for (const AliasSymbol *alias : objFile->aliases) {
+        if (const auto &aliased = symtab->find(alias->getAliasedName())) {
----------------
thevinster wrote:
> int3 wrote:
> > thakis wrote:
> > > This feels a bit inelegant. Conceptually, an alias and its target could be two map entries pointing at the same symbol – if it weren't for the fact that the alias can have more restrictive visibility :/
> > agreed heh
> If we have an object file that uses aliases and we also specify `-alias` from the command line, the object file alias will replace the one from the command line. Conceptually speaking, should it be the other way around? I haven't tested this explicitly but am interested to know this result. 
ooh, good question. let me add that test case


================
Comment at: lld/MachO/SymbolTable.cpp:120
+                                   InputFile *newFile, bool makePrivateExtern) {
+  bool isPrivateExtern = makePrivateExtern || src->privateExtern;
+  return addDefined(target, newFile, src->isec, src->value, src->size,
----------------
thevinster wrote:
> Does this mean that both the alias and aliased symbol can differ in symbol visibility? If so, might be worth adding a test for it if it's expected behavior. 
I'm not sure I understand the question. Are you asking if they can have visibilities that differ from each other? Yes, but that is already being tested...


================
Comment at: lld/MachO/SymbolTable.h:46
+  Defined *aliasDefined(Defined *src, StringRef target, InputFile *newFile,
+                        bool makePrivateExtern = false);
 
----------------
thakis wrote:
> nit: `isPrivateExtern` for consistency with previous method?
sure. I was thinking that `isPrivateExtern` sounded like it could apply to either the src or target, whereas `makePrivateExtern` is more clearly something that will be applied to the newly-created symbol. But consistency is good too


================
Comment at: lld/test/MachO/symbol-aliases.s:43
+#--- aliases.s
+.globl _extern_alias_to_strong, _extern_alias_to_weak, _extern_alias_to_weak
+
----------------
thakis wrote:
> Do you have a test for an extern alias to a local or a private extern target?
I'll add a test for a private extern target. AFAICT, MC won't generate aliases to locals (it will just refer to the aliased symbol itself), so that case can't / doesn't need to be tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133825/new/

https://reviews.llvm.org/D133825