[PATCH] D49456: [AArch64] Support execute-only LOAD segments.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Is this for compatibility with an existing option, or is this a new option? If this is new, I think `-aarch64` prefix might be undesirable -- even though "execute only" pages are currently supported by AArch64 among major ISAs, I can imagine that other ISA could support it. So a more platform-neutral naming might be better.



================
Comment at: ELF/Driver.cpp:305
+    if (Config->EMachine != EM_AARCH64)
+      error("-aarch64-execute-only is only supported on AArch64 targets.");
+
----------------
error messages shouldn't end with '.'


================
Comment at: ELF/Driver.cpp:308
+    if (Config->SingleRoRx)
+      error("-aarch64-execute-only and -no-rosegment cannot be used together.");
+  }
----------------
Ditto


================
Comment at: ELF/Options.td:246-249
+defm aarch64_execute_only: B<"aarch64-execute-only",
+    "Do not mark executable sections readable.",
+    "Mark executable sections readable.">;
+
----------------
This file is sorted alphabetically, so this should be move all the way top to the file.


================
Comment at: test/ELF/aarch64_execute_only.s:1
+// REQUIRES: aarch64
+
----------------
This test is perhaps a bit fragile because it depends on the current order of segments. How about this? You can specify the virtual address of the .text section by -Ttext=<value> and that can work as a marker.

I.e.

  ld.lld -Ttext=0xcafe0000 %t.o -o %t.exe -aarch64-execute-only
  llvm-readelf -l %t.exe | FileCheck %s

  CHECK: LOAD  {{.*}} 0x00000000cafe0000 0x00000000cafe0000 E
  CHECK-NOT: LOAD  {{.*}} 0x00000000cafe0000 0x00000000cafe0000 R E



Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49456