Page MenuHomePhabricator

[AIX][clang] Storage Locations for Constant Pointers
Needs ReviewPublic

Authored by qiongsiwu1 on Feb 16 2023, 7:29 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 marked an inline comment as done.Feb 24 2023, 2:38 PM

Gentle ping for review. Thank you!

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

Update lit test.

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

qiongsiwu1 updated this revision to Diff 501871.Thu, Mar 2, 7:50 AM

Updating -fdata-sections related comment.

Ping for review. Thanks!

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 added inline comments.
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 added inline comments.Wed, Mar 8, 9:31 AM
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 added inline comments.Wed, Mar 8, 10:04 AM
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?

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

qiongsiwu1 marked 8 inline comments as done and an inline comment as not done.Wed, Mar 8, 10:27 AM

Update -mroptr cc1 option help text.

qiongsiwu1 marked an inline comment as done.Wed, Mar 8, 10:33 AM
clang/test/Driver/ppc-roptr.c
11

Sorry, I meant on the LTO link step.

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.

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.

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 updated this revision to Diff 503796.Thu, Mar 9, 8:43 AM

Rename the ReadOnlyPointers option in the source to XCOFFReadOnlyPointers.

qiongsiwu1 updated this revision to Diff 503934.Thu, Mar 9, 2:16 PM

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

qiongsiwu1 updated this revision to Diff 503941.Thu, Mar 9, 2:55 PM

Fixing release note. Adding pure linking tests.

qiongsiwu1 marked 6 inline comments as done.Thu, Mar 9, 2:56 PM
qiongsiwu1 updated this revision to Diff 503944.Thu, Mar 9, 3:00 PM

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

qiongsiwu1 marked an inline comment as done.Thu, Mar 9, 3:00 PM
qiongsiwu1 updated this revision to Diff 503946.Thu, Mar 9, 3:13 PM

Adding LTO error check tests.

Ping for review. Thank you!

Ping for review. Thank you!

stephenpeckham added inline comments.Tue, Mar 14, 9:11 AM
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.

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

qiongsiwu1 marked 4 inline comments as done.Tue, Mar 14, 11:05 AM
qiongsiwu1 added inline comments.Wed, Mar 15, 10:13 AM
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!!

Ping for review/comments. Thanks so much!

Ping for review/comments. Thanks so much!

MaskRay added inline comments.Wed, Mar 22, 3:07 PM
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
MaskRay added inline comments.Wed, Mar 22, 3:12 PM
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
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 added inline comments.Thu, Mar 23, 8:02 AM
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)?

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.

qiongsiwu1 marked 5 inline comments as done.Thu, Mar 23, 10:14 AM
qiongsiwu1 added a reviewer: MaskRay.
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 added inline comments.Tue, Mar 28, 6:52 AM
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!

Fixing LTO data sections error check.

Simplify LTO data section error check.

qiongsiwu1 marked an inline comment as done.Tue, Mar 28, 11:51 AM
qiongsiwu1 edited the summary of this revision. (Show Details)

Renaming the -mroptr option to -mxcoff-roptr.

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.