[PATCH] D112735: export unique symbol list for xcoff with llvm-objdump new option "--export-unique-symbol"

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/SymbolicFile.h:122
                                  // (IR only)
+    SF_Protected = 1U << 12,     // Symbol has protected visibility
+    SF_Internal = 1U << 13       // Symbol has internal visibility
----------------
MaskRay wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > MaskRay wrote:
> > > > DiggerLin wrote:
> > > > > MaskRay wrote:
> > > > > > I don't think we need new bits.
> > > > > > 
> > > > > > If internal visibility has similar behavior with hidden visibility, just reuse it or not set symbol properties at all.
> > > > > > 
> > > > > > I am mostly concerned with the fact that BFD style describing all binary format's every symbol property simply does not work.
> > > > > we need the "protected" visibility in xcoff object file.
> > > > > 
> > > > > The AIX linker  accepts 4 of such visibility attribute types:
> > > > > 
> > > > > export: Symbol is exported with the global export attribute.
> > > > > hidden: Symbol is not exported.
> > > > > protected: Symbol is exported but cannot be rebound (preempted), even if runtime linking is being used.
> > > > > internal: Symbol is not exported. The address of the symbol must not be provided to other programs or shared objects, but the linker does not verify this.
> > > > > 
> > > > >  please  reference 
> > > > > 1. [[ https://www.ibm.com/docs/en/aix/7.1?topic=formats-xcoff-object-file-format | xcoff-object-file-format ]] . (search "Symbol visibility" in file". )
> > > > > 
> > > > > 2. https://developer.ibm.com/articles/au-aix-symbol-visibility/ , search 'STV_PROTECTED"
> > > > > 
> > > > Thanks for the pointers. But see my comment below: the request is about removing the unneeded `SF_*` abstraction.
> > > sorry that , I can not get "the request is about removing the unneeded SF_* abstraction"
> > > 
> > > your suggestion is to change  from "SF_Protected"  to "Protected"  ?  if so, my suggestion is that we keep "SF_Protected" as in this patch. and create a NFC to remove "SF_" 
> > > 
> > @MaskRay 
> My suggestion is to avoid the `SF_*` abstraction when implementing the dumper support. 
> 
> `exportSymbolInfoFromObjectFile` is XCOFF specific and ideally just uses the raw XCOFF interface, instead of using `SF_*` bits.
> 
> The `SF_*` bits are added prudently, not in one-shot places.
thanks for explain. @MaskRay  . as I mentioned, xcoff has 4 visibility. (Protected is  only for xcoff). 

there is common interface

```
 Expected<uint32_t> XCOFFObjectFile::getSymbolFlags(DataRefImpl Symb) const {
```
in the  
/scratch/zhijian/llvm/src/llvm/lib/Object/XCOFFObjectFile.cpp

if I implement getting visibility in the getSymbolFlags(), without the  protected visibility. it will look like

    
```
  // There is no visibility in old 32 bit XCOFF object file interpret.
  if (is64Bit() || (auxiliaryHeader32() && (auxiliaryHeader32()->getVersion() ==
                                            NEW_XCOFF_INTERPRET))) {

    uint16_t SymType = XCOFFSym.getSymbolType();
    if ((SymType & VISIBILITY_MASK) == SYM_V_INTERNAL)
      Result |= SymbolRef::SF_Internal;
    if ((SymType & VISIBILITY_MASK) == SYM_V_HIDDEN)
      Result |= SymbolRef::SF_Hidden;
    if ((SymType & VISIBILITY_MASK) == SYM_V_EXPORTED)
      Result |= SymbolRef::SF_Exported;
  }

```
if the symbo's visibility is "protected" , without defining something like "SF_Protected" in the SymbolicFile.h, it can not get the correct flag for it.    it is not reasonable for xcoff. 

I think the only thing I can do is to change the name "SF_Protected" to "XCOFF_Protected"  and add some comment for the XCOFF_Protected in the source code. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735