This is an archive of the discontinued LLVM Phabricator instance.

[AIX][clang] Storage Locations for Constant Pointers
ClosedPublic

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

qiongsiwu1 created this revision.Feb 16 2023, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 7:29 AM
qiongsiwu1 requested review of this revision.Feb 16 2023, 7:29 AM

Rebase to resolve clang release note conflict.

Gentle ping for review. Thanks!

stephenpeckham accepted this revision.Feb 23 2023, 1:14 PM

I don't have issues with this code.

This revision is now accepted and ready to land.Feb 23 2023, 1:14 PM
hubert.reinterpretcast requested changes to this revision.Feb 24 2023, 12:34 PM
hubert.reinterpretcast added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5248

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).

This revision now requires changes to proceed.Feb 24 2023, 12:34 PM

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.Mar 2 2023, 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.Mar 8 2023, 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.Mar 8 2023, 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.Mar 8 2023, 10:27 AM

Update -mroptr cc1 option help text.

qiongsiwu1 marked an inline comment as done.Mar 8 2023, 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.Mar 9 2023, 8:43 AM

Rename the ReadOnlyPointers option in the source to XCOFFReadOnlyPointers.

qiongsiwu1 updated this revision to Diff 503934.Mar 9 2023, 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.Mar 9 2023, 2:55 PM

Fixing release note. Adding pure linking tests.

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

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

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

Adding LTO error check tests.

Ping for review. Thank you!

Ping for review. Thank you!

stephenpeckham added inline comments.Mar 14 2023, 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.Mar 14 2023, 11:05 AM
qiongsiwu1 added inline comments.Mar 15 2023, 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.Mar 22 2023, 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.Mar 22 2023, 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.Mar 23 2023, 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.Mar 23 2023, 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.Mar 28 2023, 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.Mar 28 2023, 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.

clang/include/clang/Basic/DiagnosticDriverKinds.td
647

Typo.

clang/lib/Driver/ToolChains/Clang.cpp
5258–5259

I think this belongs in the code for AIX linker jobs.

clang/test/Driver/ppc-roptr.c
15

This is compile+LTO link. We can also add ROPTR,LINK prefixes here, right?

24

Please make this case pure-link. The diagnostic is most relevant on the link step because that is when -shared takes effect.

26

Please make this case pure-link; otherwise, we will pass the test simply because the diagnostic is also implemented for compiling-from-source.

28

Same comment.

30

Same comment.

38

This needs the backend option also renamed. Has a commit for that landed yet?

clang/docs/ReleaseNotes.rst
317

Two spaces => one space

clang/lib/Driver/ToolChains/CommonArgs.cpp
718–721

Should use hasFlag to check if data_sections is off.

As background:
The logic near line 699 says to add -data-sections=1 if data_sections is on and, if data_sections is off, only add -data-sections=0 if -fno-data-sections was explicitly present. This allows the LTO default for data_sections to be a separate policy from the non-LTO default.

clang/lib/Driver/ToolChains/AIX.cpp
125

Old option name in comment text.

clang/lib/Frontend/CompilerInvocation.cpp
1965

Old option name in comment text.

qiongsiwu1 updated this revision to Diff 509342.EditedMar 29 2023, 6:36 AM

Rebase and address feedback other than error checks and related test cases.

qiongsiwu1 marked 4 inline comments as done.Mar 29 2023, 6:38 AM

Moving the -shared check to the AIX linker and modifying the tests so we test pure linking.

qiongsiwu1 marked an inline comment as done.

Fix the LTO data sections check.

qiongsiwu1 marked 6 inline comments as done.Mar 29 2023, 7:43 AM
qiongsiwu1 marked an inline comment as done.
qiongsiwu1 added inline comments.
clang/test/Driver/ppc-roptr.c
38

No the commit has not landed. The patch is up https://reviews.llvm.org/D147161.

qiongsiwu1 marked an inline comment as done.Mar 29 2023, 8:39 AM
qiongsiwu1 added inline comments.Mar 31 2023, 7:47 AM
clang/test/Driver/ppc-roptr.c
38

This needs the backend option also renamed. Has a commit for that landed yet?

Thanks so much for the fast review Hubert! The commit has now landed.

qiongsiwu1 added a comment.EditedApr 3 2023, 8:17 AM

Ping for review. Specifically, the option is renamed to -mxcoff-roptr and -mno-xcoff-roptr to indicate that this option only affects the XCOFF binary format, so it is expected to not affect ELF or other formats. The implication is that it is no longer necessary for us to implement checks for pure linking jobs on non-AIX platforms, because it is expected that this XCOFF specific option does not do anything on non-AIX platforms.

Thanks!

Ping for review. Thank you!

@hubert.reinterpretcast please let me know if there are comments/suggestions! Thanks!

qiongsiwu1 updated this revision to Diff 520375.May 8 2023, 7:50 AM

Rebase. Ping for review @hubert.reinterpretcast . Thanks so much!

hubert.reinterpretcast accepted this revision.EditedMay 15 2023, 12:06 AM

LGTM (with minor comments).

clang/lib/Driver/ToolChains/AIX.cpp
132

Minor nit: needs => need

clang/lib/Driver/ToolChains/CommonArgs.cpp
718–719

Remove the comment. The hasFlag check requires no special reasoning.

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

Remove c2. Not needed for the test.

28–31

Remove main. Not needed for the test.

This revision is now accepted and ready to land.May 15 2023, 12:06 AM

Thanks so much @hubert.reinterpretcast !! The comments are addressed. I will land the patch when the pre-commit CI finishes.

qiongsiwu1 marked 4 inline comments as done.May 15 2023, 5:28 AM

Pre-commit CI x64 Debian is failing https://buildkite.com/llvm-project/premerge-checks/builds/152189 because the build machine's cmake version is too old. I am landing this patch.

This revision was landed with ongoing or failed builds.May 15 2023, 8:34 AM
This revision was automatically updated to reflect the committed changes.
qiongsiwu1 added a comment.EditedMay 15 2023, 8:50 AM

This patch is causing some buildbot failures. Specifically

https://lab.llvm.org/buildbot/#/builders/139/builds/40652
https://lab.llvm.org/buildbot/#/builders/258/builds/810
https://lab.llvm.org/buildbot/#/builders/216/builds/21208

I am looking into them and aiming at resolving all in a few hours. Please revert if this cannot wait. Thanks!

https://reviews.llvm.org/D150586 has landed and the problematic test case is removed.