[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

[Stephen Peckham via Phabricator via cfe-commits]

stephenpeckham added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:230
+- Introduced the ``-mroptr`` option to place constant objects with relocatable
+  address values in the read-only data section. This option is intended to
+  be used with the ``-fdata-sections`` option. When ``-mroptr`` is in effect
----------------
"intended" is a bit misleading.  It's an error to use -mroptr and -fno-data-sections together.


================
Comment at: clang/include/clang/Driver/Options.td:3896
+def mroptr : Flag<["-"], "mroptr">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Place constant objects with relocatable address values in the RO data section and imply -bforceimprw when specified at link time">;
+def mno_roptr : Flag<["-"], "mno-roptr">, Group<m_Group>;
----------------
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."  


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1960
+
+    // Since the stroage mapping class is specified per csect,
+    // without using data sections, it is ambiguous what exactly should
----------------
typo


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1961
+    // Since the stroage mapping class is specified per csect,
+    // without using data sections, it is ambiguous what exactly should
+    // be done for the read-only pointers. Using read-only pointers may cause
----------------
It's not ambiguous to use -fno-data-sections and -mroptr together, but it would be  less effective.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144190/new/

https://reviews.llvm.org/D144190