Page MenuHomePhabricator

add new option -mignore-xcoff-visibility
ClosedPublic

Authored by DiggerLin on Sep 10 2020, 6:39 AM.

Details

Summary

In IBM compiler xlclang , there is an option -fnovisibility which suppresses visibility. For more details see: https://www.ibm.com/support/knowledgecenter/SSGH3R_16.1.0/com.ibm.xlcpp161.aix.doc/compiler_ref/opt_visibility.html.

We need to add the option -mignore-xcoff-visibility for compatibility with the IBM AIX OS (as the option is enabled by default in AIX). With this option llvm does not emit any visibility attribute to ASM or XCOFF object file.

The option only work on the AIX OS, for other non-AIX OS using the option will report an unsupported options error. For example, the file test.c:

 bash-4.2$ test.c

 __attribute__((visibility ("protected"))) int b;
 

clang   -mignore-xcoff-visibility   -target powerpc-unknown-linux    -S test.c
clang-12: error: unsupported option '-mignore-xcoff-visibility' for target 'powerpc-unknown-linux

In AIX OS:

1.1  the option -mignore-xcoff-visibility is enabled by default , if there is not -fvisibility=* and -mignore-xcoff-visibility explicitly in the clang command .

  clang -mignore-xcoff-visibility     -target powerpc-unknown-aix    -S test.c

 or
  
  clang -target powerpc-unknown-aix    -S test.c  ( the -mignore-xcoff-visibility  is enabled by default in AIX OS)

 Generate as  as :
 
  .globl  b 

1.2 if there is -fvisibility=* explicitly but not -mignore-xcoff-visibility  explicitly in the clang command.  it will generate visibility attributes.

clang  -fvisibility=default   -target powerpc-unknown-aix    -S test.c

  Generate ASM  as :
  .globl  b,protected

1.3 if there are  both  -fvisibility=* and  -mignore-xcoff-visibility  explicitly in the clang command. The option  "-mignore-xcoff-visibility" wins , it do not emit the visibility attribute. 

clang -mignore-xcoff-visibility   -fvisibility=default  -target powerpc-unknown-aix   -S test.c

 Generate ASM as :
 .globl  b
 

The option -mignore-xcoff-visibility has no effect on visibility attribute when compile with -emit-llvm option to generated LLVM IR.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 6:39 AM
DiggerLin requested review of this revision.Sep 10 2020, 6:39 AM
DiggerLin edited the summary of this revision. (Show Details)Sep 10 2020, 6:42 AM
DiggerLin edited the summary of this revision. (Show Details)

Bad commit message - links will go dead eventually, all the relevant stuff needs to be specified in the commit message itself.

clang/docs/ClangCommandLineReference.rst
2310–2316
DiggerLin updated this revision to Diff 290978.EditedSep 10 2020, 8:08 AM

add a pragma test case

DiggerLin updated this revision to Diff 290987.Sep 10 2020, 8:37 AM

address comment

DiggerLin marked an inline comment as done.Sep 10 2020, 9:06 AM
DiggerLin edited the summary of this revision. (Show Details)Sep 10 2020, 10:25 AM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin retitled this revision from add new clang option -fnovisibility. to add new clang option -mignore-xcoff-visibility.
DiggerLin edited the summary of this revision. (Show Details)Sep 15 2020, 10:27 AM
jasonliu added inline comments.Sep 16 2020, 8:35 AM
clang/docs/ClangCommandLineReference.rst
2310

We should move this option to where all the other -m options resides.

2312
clang/include/clang/Basic/LangOptions.def
300 ↗(On Diff #291962)

nit: AIX XCOFF target

clang/include/clang/Driver/Options.td
1966

We should move this option to where the m groups belong.

clang/lib/AST/Decl.cpp
1487 ↗(On Diff #291962)

Question: When I look for visibility in CodeGen, I could find some of the visibility settings there that does not depend on the query of existing visibility settings. For example, some of the vtable generation, which means we could still get visibility settings out from IR in some situation?
Does those visibility settings in the CodeGen matters?

clang/lib/Frontend/CompilerInvocation.cpp
2773

Question: how should -mignore-xcoff-visiblity interact with -ftype-visibility?

clang/test/Driver/ignore-xcoff-visibility.cpp
25

I don't feel like this test belongs in the Driver directory.
There is driver test needed, which is to pass in -### and specify -mignore-xcoff-visibility to see if it gets passed by the driver.
But the majority of what this test tests seems belong to CodeGen directory.

63

Do we also want to test attribute visibility on a type?
For example:

class __attribute__((__visibility__("hidden"))) TestClass {
   int foo();
   int bar();
}
DiggerLin retitled this revision from add new clang option -mignore-xcoff-visibility to add new clang option -mno-xcoff-visibility.Sep 22 2020, 1:48 PM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin updated this revision to Diff 293571.Sep 22 2020, 2:42 PM

address comment

daltenty added inline comments.Sep 22 2020, 3:51 PM
llvm/include/llvm/Target/TargetMachine.h
267

This seems like it needs the corresponding comand-line option for llc added and an llc test.

DiggerLin marked an inline comment as done.Sep 23 2020, 6:09 AM
DiggerLin added inline comments.
llvm/include/llvm/Target/TargetMachine.h
267

I think it will be in another separate patch.

DiggerLin marked 5 inline comments as done.Sep 24 2020, 8:11 AM
DiggerLin added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
2773

the -ftype-visibility do not support in the clang, so there is no interact.

DiggerLin updated this revision to Diff 294070.Sep 24 2020, 8:22 AM
DiggerLin marked an inline comment as done.
jasonliu added inline comments.Sep 24 2020, 11:49 AM
clang/docs/ClangCommandLineReference.rst
2310

It's rare to see an option with only the negative form. Could we rename and make it a positive form somehow?
Also we would need to move this option to where the m group belongs.

clang/test/CodeGen/aix-no-xcoff-visibility.cpp
1 ↗(On Diff #294070)

I don't think we should call the driver directly in here.
We should have a separate driver test where we invoke clang, and we should invoke the front end clang_cc1 here.

75 ↗(On Diff #294070)

Not sure if the IR check is really necessary, since we haven't made any IR change here. It's going to be all the same with or without the new -m option.

jasonliu added inline comments.Sep 24 2020, 11:52 AM
llvm/include/llvm/Target/TargetMachine.h
267

I would actually prefer to have that in the same patch, as that would give us a full picture. It's not a huge patch even if we combine them.

DiggerLin marked an inline comment as done.Sep 24 2020, 1:31 PM
DiggerLin added inline comments.
llvm/include/llvm/Target/TargetMachine.h
267

yes, it is not huge patch, one patch for the clang with option -mno-xcoff-visibility, another patch for llc option -no-xcoff-visibility , I think it is different functionality. and I have post the https://reviews.llvm.org/D88234 "add new option -no-xcoff-visibility for llc"

daltenty added inline comments.Sep 24 2020, 2:18 PM
llvm/include/llvm/Target/TargetMachine.h
267

But the problem is this patch actually introduces the backend functionality with no test in the LLVM component itself, because the test here is Clang only. Either the patches should be merged so both components get tests in one patch or both refactored so we have one llc/LLVM patch and one clang patch.

DiggerLin edited the summary of this revision. (Show Details)Fri, Oct 2, 6:09 AM
DiggerLin retitled this revision from add new clang option -mno-xcoff-visibility to add new option ignore-xcoff-visibility.Fri, Oct 2, 7:52 AM
DiggerLin marked 5 inline comments as done.Fri, Oct 2, 8:00 AM
DiggerLin added inline comments.
clang/test/CodeGen/aix-no-xcoff-visibility.cpp
75 ↗(On Diff #294070)

IR output will not change in the new option. I think we need to check whether the IR be changed in the new option

DiggerLin updated this revision to Diff 295838.Fri, Oct 2, 8:44 AM
DiggerLin marked an inline comment as done.

address comment and merger the patch https://reviews.llvm.org/D88234 into the patch

daltenty added inline comments.Fri, Oct 2, 10:11 AM
clang/docs/ClangCommandLineReference.rst
2622

nit: add a space before parens

2622

I don't think the object file writing case was handled yet? This makes it sound like it is.

clang/lib/Driver/ToolChains/Clang.cpp
5246

Use Args.hasFlag instead, since this option doesn't have a value we need to check.

clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
73

This isn't being checked anymore, also probably belongs in the other file

clang/test/Driver/ignore-xcoff-visibility.cpp
3

We should check the diagnostic here

4

nit: We should constrain this to be following the cc1 invocation

DiggerLin marked 7 inline comments as done.Mon, Oct 5, 12:49 PM
DiggerLin added inline comments.
clang/docs/ClangCommandLineReference.rst
2622

yes, we do not implement the visibility of the object file in the XCoffObjectWriter.cpp

clang/lib/Driver/ToolChains/Clang.cpp
5246

we need to get the value here

D.Diag(diag::err_drv_unsupported_opt_for_target)
     << A->getAsString(Args) << TripleStr;
clang/test/CodeGen/aix-ignore-xcoff-visibility.cpp
73

thanks

DiggerLin updated this revision to Diff 296267.Mon, Oct 5, 12:52 PM
DiggerLin marked 3 inline comments as done.

address comment

daltenty retitled this revision from add new option ignore-xcoff-visibility to add new option -mignore-xcoff-visibility.Tue, Oct 6, 11:45 AM
daltenty edited the summary of this revision. (Show Details)
daltenty edited the summary of this revision. (Show Details)
daltenty edited the summary of this revision. (Show Details)
daltenty added inline comments.Tue, Oct 6, 1:00 PM
llvm/lib/CodeGen/CommandFlags.cpp
339

nit: match description above

daltenty added inline comments.Tue, Oct 6, 1:06 PM
clang/docs/ClangCommandLineReference.rst
2622

nit: plural. capitalization.

clang/include/clang/Driver/Options.td
2570
llvm/lib/CodeGen/CommandFlags.cpp
340

nit: capitalize XCOFF

daltenty edited the summary of this revision. (Show Details)Tue, Oct 6, 1:07 PM
daltenty accepted this revision.Tue, Oct 6, 1:14 PM

LGTM, other minor nit

This revision is now accepted and ready to land.Tue, Oct 6, 1:14 PM
jasonliu added inline comments.Wed, Oct 7, 10:28 AM
llvm/include/llvm/Target/TargetOptions.h
126–129

Should the default be true for this option?
As this is an XCOFF only option, and the default for clang is true, so it would be better for llc to match as well?

DiggerLin marked an inline comment as done.Wed, Oct 7, 12:01 PM
DiggerLin added inline comments.
llvm/include/llvm/Target/TargetOptions.h
126–129

we can implement it in another separate patch.

This revision was landed with ongoing or failed builds.Thu, Oct 8, 6:35 AM
This revision was automatically updated to reflect the committed changes.
DiggerLin marked an inline comment as done.

Hello. I added a power-pc REQUIRES clause to the new clang test here in a15bd0bfc20c2b2955c59450a67b6e8efe89c708. Hope that looks OK.