[PATCH] D33251: [lld][ELF]Add option to make .dynamic read only

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:1008
 
   // .dynamic section is not writable on MIPS.
   // See "Special Section" in Chapter 4 in the following document:
----------------
... on MIPS and on Fuchsia OS which passes `-z rodynamic`.


================
Comment at: ELF/SyntheticSections.cpp:1011
   // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-  if (Config->EMachine == EM_MIPS)
+  // Users may want to make .dynamic read-only on other systems as well.
+  if (Config->EMachine == EM_MIPS || Config->ZReadOnlyDynamic)
----------------
Probably it is better to not add this comment as it is not informative but could be confusing (e.g. why users may want it on other systems?)


================
Comment at: ELF/SyntheticSections.cpp:1057
 
-  if (!Config->Shared && !Config->Relocatable)
+  //DT_DEBUG will cause a segfault if .dynamic is read-only
+  if (!Config->Shared && !Config->Relocatable && !Config->ZReadOnlyDynamic)
----------------
Add a space after `//` and a full stop at end of the line.

But you want to extend the comment to explain the background. I'd write something like:

DT_DEBUG points to a location containing some debug info so that debuggers can find debug info at runtime. We need it for each process, so we don't write it for DSOs.

DT_DEBUG is the only .dynamic entry to which the loader is expected to write. Some systems, such as Fuchsia, chose to make .dynamic sections completely read-only (and provides alternative mean to find debug info). If the target is such system, we don't write DT_DEBUG.


Repository:
  rL LLVM

https://reviews.llvm.org/D33251