[PATCH] D84265: [AIX][XCOFF] emit symbol visibility for xcoff object file.

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:39
+      MCSect->setVisibilityType(Symbol->getVisibilityType());
+  }
+}
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Some thoughts on how to implement this for object generation:
> > If we want to follow existing practice, the closest thing we could find is `StorageClass`. 
> > Both symbol and section have storage class, but how we set them are different.
> > For sections, we set the storage class when we create them. For symbols, we set it using `emitSymbolAttribute`.
> > So if we want to follow existing practice, we want to set the visibility with `getXCOFFSection`.
> > 
> > But if we want to break the existing practice (which could be a bigger change), we could consider using `getQualNameSymbol` as proxy of the MCSection. 
> > So we only set attribute or property on MCSymbolXCOFF when possible, and in obj gen, we use `getQualNameSymbol` to get the property that we need for the section. 
> > But the problem with that is the external symbols. We use external function entry point symbols as if they are labels, but in fact, they are csects. So getQualNameSymbol for an external function entry point symbol would not return the symbol that have the correct settings on it. So maybe this is one motivation for us to change `.bl .foo` to `.bl .foo[PR]` for external symbols.
> > 
> > As it is right now, if we seeking for minimum work and keep things consistent, we could set the visibility using getXCOFFSection. And if later we think it's important enough to pursue using `getQualNameSymbol` as proxy of the MCSection, we could do a refactoring at that time?
> there are 
>   XCOFF::StorageMappingClass MappingClass;
>   XCOFF::SymbolType Type;
>   XCOFF::StorageClass StorageClass;
>   XCOFF::VisibilityType VisibilityType = XCOFF::SYM_V_UNSPECIFIED;
> in the MCSectionXCOFF  class,
> the four attribute only used in the XCOFFObjectWriter, (asm path do not need the StorageClass)
> 
> we need get the value of StorageClass, Type,MappingClass before we invoke the getXCOFFSection every time.
> 
> actually for the StorageClass, we can set the value of StorageClass when set the
> MCXCOFFStreamer::emitSymbolAttribute(). (for common link we can set at  MCXCOFFStreamer::emitCommonSymbol).
> 
> if we want to refactor StorageClass later, I do not think we need to consistent with StorageClass in this patch. we can do NFC after this patch for StorageClass.
I don't want the setting of MCSectionXCOFF to be different for different property.
So if we do not want to pursue the proxy of MCSymbolXCOFF for now, we should make them consistent. 
Or we could do the refactoring first.
This middle state is not what we want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84265