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

[David Tenty via Phabricator via llvm-commits]

daltenty marked an inline comment as done.
daltenty 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:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > 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.
> > Yes, your understanding is correct. Omitting function descriptor label symbol when it's a static function is an optimization.
> I agree this is the underlying issue. Ideally, we should be able to handle this with MCSymbol which are Temporary. That seems to be how other targets handle user written assembler temporaries. 
> 
> In this case the label here would be marked as Temporary and we would not emit it naturally in the symbol table. This would require some major restructuring of how this symbol gets created however, since this property can only be set at creation time currently.
> 
> However, we have another bit of an issue with this approach. It looks like other assembly dialects use a prefix to indicate this, and set the attribute when the symbol is created, where we use a directive so we cannot know this at the time the symbol is created. So we'd need a mechanism to set it afterwards anyway.
Summarizing offline discussion: We will use the external attribute to determine if the label should be emitted into the symbol table. In a follow on patch we will have to set that appropriately for the assembly parsing case when encountering .lgobl and family directives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74968