[PATCH] D28911: [LinkerScript] Implement `MEMORY` command

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits.

Please verify that your new tests pass with BFD linker so that LLD's behavior is the same as BFD.



================
Comment at: ELF/LinkerScript.cpp:541-542
+
+  if (!Opt.MemoryRegions.size())
+    return nullptr;
+
----------------
meadori wrote:
> ruiu wrote:
> > Do you need this? A `nullptr` will naturally be returned without this, I think.
> A `nullptr` will be returned, but the error at the bottom of the function might be triggered as well.  To avoid (1) executing nop-code on an empty region map and (2) to avoid hitting that error path I chose to do an early return.
Then can you move this piece of code to the beginning of this function to return earlier?


================
Comment at: ELF/LinkerScript.cpp:551
+  // Otherwise, no suitable region was found.
+  if ((Sec->Flags & SHF_ALLOC) != 0)
+    error("no memory region specified for section '" + Sec->Name + "'");
----------------
You can remove `!= 0` because any `if (X)` is equivalent to `if ((X) != 0)`.


================
Comment at: ELF/LinkerScript.h:199
+  uint64_t Length;
+  uint64_t Index;
+  uint32_t Flags;
----------------
I found that `Index` is a bit confusing. Maybe `Offset`?


https://reviews.llvm.org/D28911