[PATCH] D74968: [XCOFF] Handle MCSA_LGlobal in emitSymbolAttribute

[Jason Liu via Phabricator via llvm-commits]

jasonliu added a subscriber: Xiangling_L.
jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:688
 
-  if (MAI->needsFunctionDescriptors() &&
-      F.getLinkage() != GlobalValue::InternalLinkage)
+  if (MAI->needsFunctionDescriptors())
     emitLinkage(&F, CurrentFnDescSym);
----------------
I understand this is to make the object writing case and assembly case to be equivalent. 
But I think this changes the original design intent. The intent was when it is a static function, we do not need an label in the symbol table at all. (@Xiangling_L Correct me if I'm wrong.) Also I'm not sure if it actually make sense to have a label in the symbol table that is not external.
So in order to not change the design intent, I think the way to fix is the other way around. i.e although we registered a symbol, but if it is a label and is C_HIDEXT, do not add the symbol into csect's symbol list. So that we do not need to emit the internal label in the object file generation. 


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:16
 #include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCObjectWriter.h"
----------------
Do we need this include at all?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-internal.ll:8
+; Function Attrs: noinline nounwind optnone
+define internal i32 @foo() #0 {
+  ret i32 1
----------------
Remove #0


================
Comment at: llvm/test/CodeGen/PowerPC/aix-internal.ll:20
+;CHECK:  Symbol {
+;CHECK:      Name: foo
+;CHECK-NEXT: Value (RelocatableAddress):
----------------
Without the present of SymbolType, I'm not sure what are we testing here. Are we expecting a label or a csect, or both?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74968