[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]

Rebase to resolve clang release note conflict.

[qiongsiwu1]

Gentle ping for review. Thanks!

[stephenpeckham]

I don't have issues with this code.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/Clang.cpp

5244

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

[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

541–543

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.

542

Typo fix.

clang/include/clang/Driver/Options.td

3911

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

543

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

5244

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

542

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

clang/include/clang/Driver/Options.td

3911

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

1944

typo

1945

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

5244

@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

5244

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

759–761

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

759–761

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.

[hubert.reinterpretcast]

clang/include/clang/Basic/DiagnosticDriverKinds.td

653

Typo.

clang/lib/Driver/ToolChains/Clang.cpp

5254–5255

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

clang/test/Driver/ppc-roptr.c

16

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

25

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

27

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

29

Same comment.

31

Same comment.

38

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

[hubert.reinterpretcast]

clang/docs/ReleaseNotes.rst

545

Two spaces => one space

clang/lib/Driver/ToolChains/CommonArgs.cpp

763–766

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.

[hubert.reinterpretcast]

clang/lib/Driver/ToolChains/AIX.cpp

125

Old option name in comment text.

clang/lib/Frontend/CompilerInvocation.cpp

1949

Old option name in comment text.

[qiongsiwu1]

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

[qiongsiwu1]

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

[qiongsiwu1]

Fix the LTO data sections check.

[qiongsiwu1]

clang/test/Driver/ppc-roptr.c

38

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

[qiongsiwu1]

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]

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!

[qiongsiwu1]

Ping for review. Thank you!

[qiongsiwu1]

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

[qiongsiwu1]

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

[hubert.reinterpretcast]

LGTM (with minor comments).

clang/lib/Driver/ToolChains/AIX.cpp

132

Minor nit: needs => need

clang/lib/Driver/ToolChains/CommonArgs.cpp

763–764

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.

[qiongsiwu1]

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

[qiongsiwu1]

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.

[qiongsiwu1]

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!

[qiongsiwu1]

Patch to remove the problematic test case. https://reviews.llvm.org/D150586

[qiongsiwu1]

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

clang/docs/ReleaseNotes.rst

 CUDA/HIP Language Changes
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 CUDA Support
 ^^^^^^^^^^^^
 
 AIX Support
 ^^^^^^^^^^^
 - Add an AIX-only link-time option, `-mxcoff-build-id=0xHEXSTRING`, to allow users
   to embed a hex id in their binary such that it's readable by the program itself.
   This option is an alternative to the `--build-id=0xHEXSTRING` GNU linker option
   which is currently not supported by the AIX linker.
 
+- Introduced the ``-mxcoff-roptr`` option to place constant objects with
+  relocatable address values in the read-only data section. This option should
+  be used with the ``-fdata-sections`` option, and is not supported with
+  ``-fno-data-sections``. When ``-mxcoff-roptr`` is in effect at link time,
+  read-only data sections with relocatable address values that resolve to
+  imported symbols are made writable.
 
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
 AVR Support
 ^^^^^^^^^^^
 - The definition of ``USHRT_MAX`` in the freestanding ``<limits.h>`` no longer
   overflows on AVR (where ``sizeof(int) == sizeof(unsigned short)``).  The type

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(CXXCtorDtorAliases, 1, 0) ///< Emit complete ctors/dtors as linker
                                      ///< aliases to base ctors when possible.
 CODEGENOPT(DataSections      , 1, 0) ///< Set when -fdata-sections is enabled.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.
 CODEGENOPT(UniqueBasicBlockSectionNames, 1, 1) ///< Set for -funique-basic-block-section-names,
                                                ///< Produce unique section names with
                                                ///< basic block sections.
 CODEGENOPT(EnableAIXExtendedAltivecABI, 1, 0) ///< Set for -mabi=vec-extabi. Enables the extended Altivec ABI on AIX.
+CODEGENOPT(XCOFFReadOnlyPointers, 1, 0) ///< Set for -mxcoff-roptr.
 ENUM_CODEGENOPT(FramePointer, FramePointerKind, 2, FramePointerKind::None) /// frame-pointer: all,non-leaf,none
 
 CODEGENOPT(ClearASTBeforeBackend , 1, 0) ///< Free the AST before running backend code generation. Only works with -disable-free.
 CODEGENOPT(DisableFree       , 1, 0) ///< Don't free memory.
 CODEGENOPT(DiscardValueNames , 1, 0) ///< Discard Value Names from the IR (LLVMContext flag)
 CODEGENOPT(DisableLLVMPasses , 1, 0) ///< Don't run any LLVM IR passes to get
                                      ///< the pristine IR generated by the
                                      ///< frontend.

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_libstdcxx_not_found : Warning<
   InGroup<DiagGroup<"stdlibcxx-not-found">>;
 
 def err_drv_cannot_mix_options : Error<"cannot specify '%1' along with '%0'">;
 
 def err_drv_invalid_object_mode : Error<
   "OBJECT_MODE setting %0 is not recognized and is not a valid setting">;
 
 def err_aix_unsupported_tls_model : Error<"TLS model '%0' is not yet supported on AIX">;
+def err_roptr_requires_data_sections: Error<"-mxcoff-roptr is supported only with -fdata-sections">;
+def err_roptr_cannot_build_shared: Error<"-mxcoff-roptr is not supported with -shared">;
 
 def err_invalid_cxx_abi : Error<"invalid C++ ABI name '%0'">;
 def err_unsupported_cxx_abi : Error<"C++ ABI '%0' is not supported on target triple '%1'">;
 
 def note_cc1_round_trip_original : Note<"original arguments in round-trip: %0">;
 def note_cc1_round_trip_generated : Note<
   "generated arguments #%0 in round-trip: %1">;
 def remark_cc1_round_trip_generated : Remark<

clang/include/clang/Driver/Options.td

 def mprivileged : Flag<["-"], "mprivileged">,
     Group<m_ppc_Features_Group>;
 def maix_struct_return : Flag<["-"], "maix-struct-return">,
   Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Return all structs in memory (PPC32 only)">;
 def msvr4_struct_return : Flag<["-"], "msvr4-struct-return">,
   Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Return small structs in registers (PPC32 only)">;
+def mxcoff_roptr : Flag<["-"], "mxcoff-roptr">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Place constant objects with relocatable address values in the RO data section and add -bforceimprw to the linker flags (AIX only)">;
+def mno_xcoff_roptr : Flag<["-"], "mno-xcoff-roptr">, Group<m_Group>;
 
 def mvx : Flag<["-"], "mvx">, Group<m_Group>;
 def mno_vx : Flag<["-"], "mno-vx">, Group<m_Group>;
 
 defm zvector : BoolFOption<"zvector",
   LangOpts<"ZVector">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable System z vector language extension">,
   NegFlag<SetFalse>>;

clang/lib/CodeGen/BackendUtil.cpp

static bool initTargetOptions(DiagnosticsEngine &Diags,
   Options.EmitCallSiteInfo = CodeGenOpts.EmitCallSiteInfo;
   Options.EnableAIXExtendedAltivecABI = LangOpts.EnableAIXExtendedAltivecABI;
   Options.XRayOmitFunctionIndex = CodeGenOpts.XRayOmitFunctionIndex;
   Options.LoopAlignment = CodeGenOpts.LoopAlignment;
   Options.DebugStrictDwarf = CodeGenOpts.DebugStrictDwarf;
   Options.ObjectFilenameForDebug = CodeGenOpts.ObjectFilenameForDebug;
   Options.Hotpatch = CodeGenOpts.HotPatch;
   Options.JMCInstrument = CodeGenOpts.JMCInstrument;
+  Options.XCOFFReadOnlyPointers = CodeGenOpts.XCOFFReadOnlyPointers;
 
   switch (CodeGenOpts.getSwiftAsyncFramePointer()) {
   case CodeGenOptions::SwiftAsyncFramePointerKind::Auto:
     Options.SwiftAsyncFramePointer =
         SwiftAsyncFramePointerMode::DeploymentBased;
     break;
 
   case CodeGenOptions::SwiftAsyncFramePointerKind::Always:

clang/lib/Driver/ToolChains/AIX.cpp

if (Args.hasArg(options::OPT_static))
     CmdArgs.push_back("-bnso");
 
   // Add options for shared libraries.
   if (Args.hasArg(options::OPT_shared)) {
     CmdArgs.push_back("-bM:SRE");
     CmdArgs.push_back("-bnoentry");
   }
 
+  if (Args.hasFlag(options::OPT_mxcoff_roptr, options::OPT_mno_xcoff_roptr,
+                   false)) {
+    if (Args.hasArg(options::OPT_shared))
+      D.Diag(diag::err_roptr_cannot_build_shared);
+
+    // The `-mxcoff-roptr` option places constants in RO sections as much as
+    // possible. Then `-bforceimprw` changes such sections to RW if they contain
+    // imported symbols that need to be resolved.
+    CmdArgs.push_back("-bforceimprw");
+  }
+
   // PGO instrumentation generates symbols belonging to special sections, and
   // the linker needs to place all symbols in a particular section together in
   // memory; the AIX linker does that under an option.
   if (Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
                     false) ||
        Args.hasFlag(options::OPT_fprofile_generate,
                     options::OPT_fno_profile_generate, false) ||
        Args.hasFlag(options::OPT_fprofile_generate_EQ,

clang/lib/Driver/ToolChains/Clang.cpp

#endif
   if (Arg *A = Args.getLastArg(options::OPT_mlong_double_128)) {
     // Emit the unsupported option error until the Clang's library integration
     // support for 128-bit long double is available for AIX.
     if (Triple.isOSAIX())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getSpelling() << RawTriple.str();
   }
 
+  if (Args.hasArg(options::OPT_mxcoff_roptr) ||
+      Args.hasArg(options::OPT_mno_xcoff_roptr)) {
+    bool HasRoptr = Args.hasFlag(options::OPT_mxcoff_roptr,
+                                 options::OPT_mno_xcoff_roptr, false);
+    StringRef OptStr = HasRoptr ? "-mxcoff-roptr" : "-mno-xcoff-roptr";
+    if (!Triple.isOSAIX())
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << OptStr << RawTriple.str();
+
+    if (HasRoptr)
+      CmdArgs.push_back("-mxcoff-roptr");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_Wframe_larger_than_EQ)) {
     StringRef V = A->getValue(), V1 = V;
     unsigned Size;
     if (V1.consumeInteger(10, Size) || !V1.empty())
       D.Diag(diag::err_drv_invalid_argument_to_option)
           << V << A->getOption().getName();
     else
       CmdArgs.push_back(Args.MakeArgString("-fwarn-stack-size=" + V));

clang/lib/Driver/ToolChains/CommonArgs.cpp

#endif
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections))
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-data-sections=1"));
   else if (Args.hasArg(options::OPT_fno_data_sections))
     CmdArgs.push_back(
         Args.MakeArgString(Twine(PluginOptPrefix) + "-data-sections=0"));
 
+  if (Args.hasArg(options::OPT_mxcoff_roptr) ||
+      Args.hasArg(options::OPT_mno_xcoff_roptr)) {
+    bool HasRoptr = Args.hasFlag(options::OPT_mxcoff_roptr,
+                                 options::OPT_mno_xcoff_roptr, false);
+    StringRef OptStr = HasRoptr ? "-mxcoff-roptr" : "-mno-xcoff-roptr";
+
+    if (!IsOSAIX)
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << OptStr << ToolChain.getTriple().str();
+
+    if (HasRoptr) {
+      if (!Args.hasFlag(options::OPT_fdata_sections,
+                        options::OPT_fno_data_sections, UseSeparateSections))
+        D.Diag(diag::err_roptr_requires_data_sections);
+
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine(PluginOptPrefix) + "-mxcoff-roptr"));
+    }
+  }
+
   // Pass an option to enable split machine functions.
   if (auto *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
                                 options::OPT_fno_split_machine_functions)) {
     if (A->getOption().matches(options::OPT_fsplit_machine_functions))
       CmdArgs.push_back(Args.MakeArgString(Twine(PluginOptPrefix) +
                                            "-split-machine-functions"));
   }
 

clang/lib/Frontend/CompilerInvocation.cpp

if (Opts.StructReturnConvention == CodeGenOptions::SRCK_OnStack) {
     OptSpecifier Opt =
         T.isPPC32() ? OPT_msvr4_struct_return : OPT_freg_struct_return;
     GenerateArg(Args, Opt, SA);
   }
 
   if (Opts.EnableAIXExtendedAltivecABI)
     GenerateArg(Args, OPT_mabi_EQ_vec_extabi, SA);
 
+  if (Opts.XCOFFReadOnlyPointers)
+    GenerateArg(Args, OPT_mxcoff_roptr, SA);
+
   if (!Opts.OptRecordPasses.empty())
     GenerateArg(Args, OPT_opt_record_passes, Opts.OptRecordPasses, SA);
 
   if (!Opts.OptRecordFormat.empty())
     GenerateArg(Args, OPT_opt_record_format, Opts.OptRecordFormat, SA);
 
   GenerateOptimizationRemark(Args, SA, OPT_Rpass_EQ, "pass",
                              Opts.OptimizationRemark);
if (O.matches(OPT_fpcc_struct_return) ||
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_OnStack);
     } else {
       assert(O.matches(OPT_freg_struct_return) ||
              O.matches(OPT_msvr4_struct_return));
       Opts.setStructReturnConvention(CodeGenOptions::SRCK_InRegs);
     }
   }
 
+  if (Arg *A = Args.getLastArg(OPT_mxcoff_roptr)) {
+    if (!T.isOSAIX())
+      Diags.Report(diag::err_drv_unsupported_opt_for_target)
+          << A->getSpelling() << T.str();
+
+    // Since the storage mapping class is specified per csect,
+    // without using data sections, it is less effective to use read-only
+    // pointers. Using read-only pointers may cause other RO variables in the
+    // same csect to become RW when the linker acts upon `-bforceimprw`;
+    // therefore, we require that separate data sections
+    // are used when `-mxcoff-roptr` is in effect. We respect the setting of
+    // data-sections since we have not found reasons to do otherwise that
+    // overcome the user surprise of not respecting the setting.
+    if (!Args.hasFlag(OPT_fdata_sections, OPT_fno_data_sections, false))
+      Diags.Report(diag::err_roptr_requires_data_sections);
+
+    Opts.XCOFFReadOnlyPointers = true;
+  }
+
   if (Arg *A = Args.getLastArg(OPT_mabi_EQ_quadword_atomics)) {
     if (!T.isOSAIX() || T.isPPC32())
       Diags.Report(diag::err_drv_unsupported_opt_for_target)
         << A->getSpelling() << T.str();
   }
 
   bool NeedLocTracking = false;
 

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

+// RUN: %clang_cc1 -triple=powerpc-ibm-aix-xcoff -mxcoff-roptr -fdata-sections \
+// RUN:     -S <%s | FileCheck %s --check-prefix=CHECK32
+// RUN: %clang_cc1 -triple=powerpc64-ibm-aix-xcoff -mxcoff-roptr -fdata-sections \
+// RUN:     -S <%s | FileCheck %s --check-prefix=CHECK64
+// RUN: not %clang_cc1 -triple=powerpc-ibm-aix-xcoff -mxcoff-roptr \
+// RUN:     -S <%s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: not %clang_cc1 -triple=powerpc64-ibm-aix-xcoff -mxcoff-roptr \
+// RUN:     -S <%s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: not %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -mxcoff-roptr \
+// RUN:     %s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+
+char c1 = 10;
+char* const c1_ptr = &c1;
+// CHECK32:         .csect c1_ptr[RO],2
+// CHECK32-NEXT:  .globl  c1_ptr[RO]
+// CHECK32-NEXT:  .align  2
+// CHECK32-NEXT:  .vbyte  4, c1[RW]
+
+// CHECK64:         .csect c1_ptr[RO],3
+// CHECK64-NEXT:  .globl  c1_ptr[RO]
+// CHECK64-NEXT:  .align  3
+// CHECK64-NEXT:  .vbyte  8, c1[RW]
+
+// DATA_SECTION_ERR: error: -mxcoff-roptr is supported only with -fdata-sections
+// TARGET_ROPTR_ERR: error: unsupported option '-mxcoff-roptr' for target 'powerpc64le-unknown-linux-gnu'

clang/test/Driver/ppc-roptr.c

+// RUN: %clang -### --target=powerpc-ibm-aix-xcoff -mxcoff-roptr %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefixes=ROPTR,LINK
+// RUN: %clang -### --target=powerpc-ibm-aix-xcoff -c -mxcoff-roptr %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=ROPTR
+// RUN: %clang -### --target=powerpc-ibm-aix-xcoff -mxcoff-roptr -mno-xcoff-roptr %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=NO_ROPTR
+
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefixes=ROPTR,LINK
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -S -mxcoff-roptr %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=ROPTR
+// RUN: %clang -### --target=powerpc-ibm-aix-xcoff %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=NO_ROPTR
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -flto %s 2>&1 | \
+// RUN:     FileCheck %s --check-prefixes=ROPTR,LINK,LTO_ROPTR
+// RUN: touch %t.o
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr %t.o 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=LINK
+
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mxcoff-roptr \
+// RUN:     %s 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mno-xcoff-roptr \
+// RUN:     %s 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
+// RUN: touch %t.o
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -shared \
+// RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=SHARED_ERR
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mxcoff-roptr -flto \
+// RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=TARGET_ROPTR_ERR
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mxcoff-roptr -flto -fno-data-sections \
+// RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR
+// RUN: %clang -### --target=powerpc64-ibm-aix-xcoff -mno-xcoff-roptr -flto -fno-data-sections \
+// RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=NO_DATA_SECTION_ERR
+// RUN: %clang -### --target=powerpc64le-unknown-linux-gnu -mno-xcoff-roptr -flto \
+// RUN:     %t.o 2>&1 | FileCheck %s --check-prefix=TARGET_NOROPTR_ERR
+
+// ROPTR: "-mxcoff-roptr"
+// LINK: "-bforceimprw"
+// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr"
+// NO_ROPTR-NOT: "-mxcoff-roptr"
+// NO_ROPTR-NOT: "-bforceimprw"
+
+// DATA_SECTION_ERR: error: -mxcoff-roptr is supported only with -fdata-sections
+// NO_DATA_SECTION_ERR-NOT: error: -mxcoff-roptr is supported only with -fdata-sections
+// TARGET_ROPTR_ERR: error: unsupported option '-mxcoff-roptr' for target 'powerpc64le-unknown-linux-gnu'
+// TARGET_NOROPTR_ERR: error: unsupported option '-mno-xcoff-roptr' for target 'powerpc64le-unknown-linux-gnu'
+// SHARED_ERR: error: -mxcoff-roptr is not supported with -shared