[PATCH] D45508: Implement --ctors-in-init-array.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu marked 2 inline comments as done.
ruiu added inline comments.


================
Comment at: lld/ELF/OutputSections.cpp:331
 
-static bool isCrtbegin(StringRef S) { return isCrtBeginEnd(S, "crtbegin"); }
-static bool isCrtend(StringRef S) { return isCrtBeginEnd(S, "crtend"); }
+bool elf::isCrtbegin(StringRef S) { return isCrtBeginEnd(S, "crtbegin"); }
+bool elf::isCrtend(StringRef S) { return isCrtBeginEnd(S, "crtend"); }
----------------
grimar wrote:
> Should these two just be inlined instead?
No I don't think so.


================
Comment at: lld/ELF/SyntheticSections.cpp:92
+// Section names may include priorities, e.g. .ctors.30 or .init_array.101.
+// .ctors.65535 is the highest priority while .init_array.0 is the highest.
+// So we need to translate the number.
----------------
grimar wrote:
> grimar wrote:
> > I would write "while .init_array.65535 is the lowest"
> My rewording was not entirely correct. Probably it could be the following:
> 
> ```
> .ctors.0 is the lowest priority while .init_array.0 is the highest.
> ```
I don't think that's a better comment because it doesn't say anything about what is the highest value. Is that 100, 255 or some other value?


================
Comment at: lld/ELF/SyntheticSections.cpp:103
+  if (S.startswith(".ctors.")) {
+    int N = 0;
+    to_integer(S.substr(7), N, 10);
----------------
grimar wrote:
> unsigned?
In general, you shouldn't use `unsigned` if the only reason to do that is a number cannot be negative, but in this particular case, I think uint16_t is the best choice.


================
Comment at: lld/ELF/SyntheticSections.cpp:112
+    return Saver.save(".fini_array." + Twine(65535 - N));
+  }
+
----------------
grimar wrote:
> Since you do not pass anything else than
> ```
> ".ctors" / ".ctors." / ".dtors" / ".dtors."
> ```
> (from `createInitFiniSections`)
> 
> 
> The code could probably be something like:
> 
> ```
> static StringRef toInitFiniName(StringRef S, uint32_t Type) {
>   StringRef Prefix = (Type == SHT_INIT_ARRAY) ? ".init_array." : ".fini_array.";
>   if (S.back() != '.')
>     return Saver.save(Prefix + "65537");
> 
>   unsigned N = 0;
>   if (to_integer(S.substr(7), N, 10))
>     error("eeerrr");
>   return Saver.save(Prefix + Twine(65535 - N));
> }
> ```
> 
> 
> 
> 
You should avoid extra allocation, but I changed the code along with that direction.


================
Comment at: lld/ELF/Writer.cpp:403
+static void createInitFiniSections() {
+  for (size_t I = 0; I < InputSections.size(); ++I) {
+    InputSection *Sec = dyn_cast<InputSection>(InputSections[I]);
----------------
grimar wrote:
> I would probably use
> 
> `for (InputSectionBase *&Base : InputSections)`
> 
> because the presence of `I` makes me search around where it is used as an index.
I disagree. A reference to a pointer to something is harder to understand than an just integer.


https://reviews.llvm.org/D45508