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

[Jason Liu via Phabricator via llvm-commits]

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);
----------------
daltenty wrote:
> jasonliu wrote:
> > 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. 
> Hmm, I do agree this does diverge from the original design intent, though I'm not sure that is avoidable for reasons outlined below.
> 
> With regard to whether it actually makes sense to have a label in the symbol table that is not external, this seems to be the entire intent of the [the .lgobl directive](https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/assembler/idalangref_lgobal_pseudo_op.html). The directive seems to be for statics that need to be kept in the symbol table (I'm assuming for relocation and other purposes). For that reason, I don't think it makes sense to eliminate them in the XCOFFObjectWriter as proposed.
> 
> Given that we can't just eliminate them in the XCOFFObjectWriter. I think we have two options: 
> 
> 1. Change the fact that we don't emit the lgobl on the label in the assembly path. Then we will be consistent between the object writing and assembly writing paths.  Though it does diverge from the original design and what both xl and gcc do, the extra label in the symbol table should be fairly harmless.
> 
> 2. Perhaps it would be less confusing to eliminate this particular label altogether? We ran into difficulties with it with the TOC as well. We already have the csect symbol which we can use in the place of this symbol in most places. This might need some larger refactoring for places we try to get a function's symbol though.
> 
> 
The two options listed are more like a workaround to the problem.
I think the real issue underneath is that, as an 
integrated-assembler, we should be able to handle case like
1.
.csect a[xx]
b:
...

Label b does not appear in the symbol table.

2. 
.globl/.lglobl/.weak/.extern b
.csect a[xx]
b:
...

b should appear in the symbol table.

But right now, I don't think we could handle that. And we might want to fix this instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74968