[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

[Digger via Phabricator via cfe-commits]

DiggerLin added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > DiggerLin wrote:
> > > > sfertile wrote:
> > > > > DiggerLin wrote:
> > > > > > jasonliu wrote:
> > > > > > > DiggerLin wrote:
> > > > > > > > DiggerLin wrote:
> > > > > > > > > jasonliu wrote:
> > > > > > > > > > DiggerLin wrote:
> > > > > > > > > > > jasonliu wrote:
> > > > > > > > > > > > DiggerLin wrote:
> > > > > > > > > > > > > jasonliu wrote:
> > > > > > > > > > > > > > Instead of just removing this line, should this get replaced with the new LangOpts option?
> > > > > > > > > > > > > I do not think we need a CodeGenOp of ignore-xcoff-visibility in clang, we only need the LangOpt of the ignore-xcoff-visilbity to control whether we will  generate the visibility in the IR,  when the LangOpt of ignore-xcoff-visibility do not generate the visibility attribute of GV in the IR. it do not need CodeGenOp of ignore-xcoff-visibility any more for the clang .
> > > > > > > > > > > > > 
> > > > > > > > > > > > > we have still CodeGen ignore-xcoff-visibility op in  llc.
> > > > > > > > > > > > We removed the visibility from IR level with this patch. But there is also visibility settings coming from CodeGen part of clang, which needs to get ignore when we are doing the code gen in llc. So I think you still need to set the options correct for llc.
> > > > > > > > > > > yes we have the set the options correct for llc in the code.
> > > > > > > > > > > 
> > > > > > > > > > > in the source file llvm/lib/CodeGen/CommandFlags.cpp, we have (in the patch https://reviews.llvm.org/D87451 add new option -mignore-xcoff-visibility) , the function
> > > > > > > > > > > TargetOptions codegen::InitTargetOptionsFromCodeGenFlags() {
> > > > > > > > > > > ....
> > > > > > > > > > > Options.IgnoreXCOFFVisibility = getIgnoreXCOFFVisibility(); 
> > > > > > > > > > > ...}
> > > > > > > > > > > 
> > > > > > > > > > What I'm saying is... 
> > > > > > > > > > I think we need a line like this:
> > > > > > > > > > `Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;`
> > > > > > > > > > so that when you invoke clang, backend would get the correct setting as well. 
> > > > > > > > > I do not think so, from the clang FE, we do not generated the visibility in the IR. so there is no need these line.
> > > > > > > > or we can say that because we do not set the hidden visibility into the GlobalValue , so we do not need the 
> > > > > > > > Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;
> > > > > > > I think I mentioned this before, we could have extra visibility settings in clang/lib/CodeGen that's not depending on the existing visibility setting in the IR. (You could search for `setVisibility` there.) That was the reason we did it in llc first. 
> > > > > > I will add Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;  here.
> > > > > > I think I mentioned this before, we could have extra visibility settings in clang/lib/CodeGen that's not depending on the existing visibility setting in the IR. (You could search for setVisibility there.) That was the reason we did it in llc first.
> > > > > 
> > > > >  A lot of these are in places we wouldn't encounter with AIX, like for Objective-C code gen. But are others like [[ https://github.com/llvm/llvm-project/blob/b03ea054db1bcf9452b3a70e21d3372b6e58759a/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2507 | this]]  an issue? Should they be addressed in this patch?
> > > > after I added the Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility , even there is  GV->setVisibility(llvm::GlobalValue::HiddenVisibility);  it do not effect our output.
> > > > 
> > > > there is following code in the function void PPCAIXAsmPrinter::emitLinkage(const GlobalValue *GV,
> > > >                                    MCSymbol *GVSym) const
> > > > {
> > > >  ..... 
> > > >   if (!TM.getIgnoreXCOFFVisibility()) {
> > > >     switch (GV->getVisibility()) {
> > > > 
> > > >     // TODO: "exported" and "internal" Visibility needs to go here.
> > > >     case GlobalValue::DefaultVisibility:
> > > >       break;
> > > >     case GlobalValue::HiddenVisibility:
> > > >       VisibilityAttr = MAI->getHiddenVisibilityAttr();
> > > >       break;
> > > >     case GlobalValue::ProtectedVisibility:
> > > >       VisibilityAttr = MAI->getProtectedVisibilityAttr();
> > > >       break;
> > > >     }
> > > >   }
> > > > 
> > > > ...
> > > > }
> > > > it do not effect our output.
> > > It can if we set the GlobalValue to be dso_local though ... that is the whole point of this patch.
> > can you give me the example of when the GlobalValue will be set to dso_local when there is a visibility been ignore in AIX?
> Just noting that setting dso_local (for reasons other than visibility) when visibility is being ignored on AIX is something that should be possible (e.g., for Extended Operations).
1. discussed with sean offline, the we do not call the emitGlobalDtorWithCXAAtExit() in AIX. so we do not have the problem for the "__dso_handle" 

2.  for the visibility,  if we ignore the visibility , we  do not set it to default visibility( the visibility will be set to unspecific ) when we output assembly or xcoff objectfile  by llc . And for the dso file, the unspecific symbol will not be exported , and the exported symbols of dso will be depend on the export list file. So I do not think there is problem for the compiler generated symbol when we ignore visibility.
for example. when there is compiler generated symbol _ _gc_log  in a  asm file .
.globl _ _gc_log
when the aix as generated the xcoff object file, the visibility of the symbol will be unspecific , not default.
only the following asm has default visibility.
.globl _ _gc_log, exported


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986