[AIX][clang] Storage Locations for Constant Pointers

[qiongsiwu1]

Summary

This patch adds clang options -mxcoff-roptr and -mno-xcoff-roptr to specify storage locations for constant pointers on AIX.

When the -mxcoff-roptr option is in effect, constant pointers, virtual function tables, and virtual type tables are placed in read-only storage. When the -mno-xcoff-roptr option is in effect, pointers, virtual function tables, and virtual type tables are placed are placed in read/write storage.

This patch depends on https://reviews.llvm.org/D144189.

[qiongsiwu1]

This accepts -mno-roptr for other platforms despite having no semantic functionality (e.g., it does not move variables to a different section for ELF codegen).

Thanks for catching this @hubert.reinterpretcast !! The patch is updated.

[qiongsiwu1]

Gentle ping for review. Thank you!

[qiongsiwu1]

Updating the release note and adding comments on the -fdata-sections requirement.

[qiongsiwu1]

Update lit test.

[qiongsiwu1]

Gentle ping for review. @hubert.reinterpretcast Thanks so much!

[qiongsiwu1]

Updating -fdata-sections related comment.

[qiongsiwu1]

Ping for review. Thanks!

[hubert.reinterpretcast]

clang/docs/ReleaseNotes.rst

313–315

Should also mention the link-time behaviour that causes the read-only data sections with relocatable address values that resolve to imported symbols to be made writable.

314

Typo fix.

clang/include/clang/Driver/Options.td

3898

Also: "Implies -bforceimprw when specified at link time."

clang/lib/Driver/ToolChains/AIX.cpp

127

Minor nit: grammar

[hubert.reinterpretcast]

clang/test/CodeGen/PowerPC/aix-roptr.c

10–11

Note that a driver test is needed for this sort of case as well.

12–17

These are driver tests. Please move (and use -### to avoid accidental reliance on front-end processing).

clang/test/Driver/ppc-roptr.c

2

Should test for -c and -S as well.
Should test for pure link (without compile) as well.

4

I think the prefix for this should be named NO_ROPTR.

11

Do we pass the option through to the LTO codegen when -fprofile-generate is used?
We may need a driver diagnostic for -mroptr without data sections when linking LTO objects (because the front-end won't be involved).

@w2yehia, does LLVM trunk have a gap in LTO driver enablement for AIX?

13–19

Why have any code here?

[w2yehia]

clang/test/Driver/ppc-roptr.c

11

does LLVM trunk have a gap in LTO driver enablement for AIX?

I think trunk is up to date in terms of LTO (driver and libLTO); this was done in LLVM 16.0.

Do we pass the option through to the LTO codegen when -fprofile-generate is used?

what's special with -fprofile-generate in relation to the ROPTR option?

[qiongsiwu1]

clang/test/Driver/ppc-roptr.c

2

Should test for pure link (without compile) as well.

Is there a set of options that I can use to trigger pure linking? Or should I use two .o input files to trigger pure linking?

[qiongsiwu1]

Addressing review comments - fixing typos, and updating the tests.

[qiongsiwu1]

Update -mroptr cc1 option help text.

[hubert.reinterpretcast]

clang/test/Driver/ppc-roptr.c

11

Sorry, I meant on the LTO link step.

[hubert.reinterpretcast]

clang/docs/ReleaseNotes.rst

315

Suggestion:
is in effect<ins> at link time</ins>,

clang/test/CodeGen/PowerPC/aix-roptr.c

10–11

-mroptr with a wrong target is currently also a front-end diagnostic (which we should then be testing here).

Having such a front-end diagnostic makes sense to me because the -cc1 option may get silently ignored otherwise: the back-end proper does not ensure that it is unset for other targets (instead llc has the checking).

clang/test/Driver/ppc-roptr.c

2

Just one .o file will trigger pure linking. For example, some tests do a touch %t.o.

7

May also want to test that the default behaviour is NO_ROPTR.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/Clang.cpp

5248

This only checks for -m[no-]roptr when the front-end is invoked. When the driver is used just for linking, we get no diagnostic at all for using these options on other platforms.

@MaskRay:

As Clang code owner for the Driver, are you concerned if platform-specific options are sometimes silently ignored on other platforms? Is there a better place for the diagnostic?

For info, in this case, the option would have effects on AIX at both compile-from-C/C++ time and at link time.

Also, should -Wunused-command-line-argument warnings be generated for cases where the option has no effect (e.g., -emit-llvm or -fsyntax-only)? I think -flto is a bit of an exception because it may be unfriendly to expect users to change other aspects of their compile invocations when adding -flto. At the same time, it can be argued instead that generating a -Wunused-command-line-argument warning for "back-end" options on -flto compiles helps the user realize that they need to provide the option on their link step.

[hubert.reinterpretcast]

clang/test/Driver/ppc-roptr.c

11

It seems to me that we don't pass the option through to the LTO codegen on the LTO link step (which is a problem).

It also seems that there is no diagnostic generated (would likely hit the back-end assertion/error) when -fno-data-sections and -mroptr is specified at the same time for the LTO link step. I believe a driver diagnostic needs to be added.

I am of the opinion that we don't have to have a front-end diagnostic in addition to a driver diagnostic if the back-end will error out in the case where relevant codegen is encountered (it provides an esoteric way to quickly check if relevant codegen is involved for a given source file). We should apply the same policy between clang -cc1 and llc though.

[qiongsiwu1]

Rename the ReadOnlyPointers option in the source to XCOFFReadOnlyPointers.

[qiongsiwu1]

Adding logic to pass mroptr to the backend during LTO codegen. Error check is not ideal.

[qiongsiwu1]

Fixing release note. Adding pure linking tests.

[qiongsiwu1]

Adding tests to check that the default behaviour is identical with -mno-roptr.

[qiongsiwu1]

Adding LTO error check tests.

[qiongsiwu1]

Ping for review. Thank you!

[qiongsiwu1]

Ping for review. Thank you!

[stephenpeckham]

clang/docs/ReleaseNotes.rst

314

"intended" is a bit misleading. It's an error to use -mroptr and -fno-data-sections together.

clang/include/clang/Driver/Options.td

3898

Should you add "(AIX only)"? Also, I don't think "imply" is a good choice here. I would say: ... and add -bforceimprw to the linker flags."

clang/lib/Frontend/CompilerInvocation.cpp

1960

typo

1961

It's not ambiguous to use -fno-data-sections and -mroptr together, but it would be less effective.

[qiongsiwu1]

Thanks for the comments @stephenpeckham ! This update addresses all of them.

[qiongsiwu1]

clang/lib/Driver/ToolChains/Clang.cpp

5248

@MaskRay May I ask for your input on this issue where platform-specific options are silently ignored on other platforms? What is our current opinion on where these checks/diagnostics should go?

Thanks!!

[qiongsiwu1]

Ping for review/comments. Thanks so much!

[qiongsiwu1]

Ping for review/comments. Thanks so much!

[MaskRay]

clang/lib/Driver/ToolChains/AIX.cpp

125

Delete // -mroptr` implies the -bforceimprw linker option.` It just repeats what the code does. Comments focus more on why/how and less on what (unless the code is very complex and explaining "what" from a high level helps).

130

drop braces https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[MaskRay]

clang/test/Driver/ppc-roptr.c

2

Prefer --target= to -target for new tests.

22

I feel that there are too many RUN lines. Every little makes. Can you reduce the numbers? For example

In many cases it isn't necessary to test every combination with both -S/-c. Testing one suffices.

When you test 4 combinations of two factors, e,g.

%clang a c
%clang a d
%clang b c
%clang b d

In many times two RUN lines are sufficient:

%clang a c
%clang b d

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/Clang.cpp

5248

Hi @MaskRay,

Thank you for your comments so far. I am particularly interested diagnostics for cases where the option does not take effect. We are hoping for a good approach to implement the diagnostic when the option is used on non-AIX platforms (without the current issue that we only check when the front-end is invoked). Additionally, I would like your opinion on whether a diagnostic is warranted for cases like -emit-llvm or -fsyntax-only. What existing command-line options are you aware of that can serve as a good design template that we can apply here?

Thanks again,
HT

[qiongsiwu1]

clang/test/Driver/ppc-roptr.c

22

Thanks for the comment!

@hubert.reinterpretcast do you think it is sufficient to test -c only (removing the -S tests) and to test powerpc64 only (removing the powerpc targets)?

[qiongsiwu1]

Address code review. Thanks for your comments/suggestions @MaskRay !!

• Fixing comments.
• Using --target= in tests.
• Reducing number of RUN lines in tests.

@hubert.reinterpretcast I ended up having a mix of tests for 32 and 64 bit targets as @MaskRay suggested.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/CommonArgs.cpp

714–716

I think this (undesirably) generates an error even -mno-roptr is in effect.

This logic seems otherwise convoluted. I think the main logic should only care that data-sections is off. We can separately assert that data-sections is on by default on AIX.

[qiongsiwu1]

clang/lib/Driver/ToolChains/CommonArgs.cpp

714–716

Thanks for catching the error! I am updating the patch to fix the undesirable error.

The checking logic is to make sure we are consistent with how we set -data-sections=0 above (line 705). I will revise the logic to check no_data_sections if we are ok with the inconsistency. @hubert.reinterpretcast could you confirm? Thanks!

[qiongsiwu1]

Fixing LTO data sections error check.

[qiongsiwu1]

Simplify LTO data section error check.

[qiongsiwu1]

Renaming the -mroptr option to -mxcoff-roptr.

[qiongsiwu1]

The new option is renamed to -mxcoff-roptr to avoid confusions if we are using it during a pure linking job on non-AIX platforms. The option name now implies that it applies to XCOFF only, so silently ignoring the option on other platforms is not that serious a problem.