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

[Meador Inge via Phabricator via llvm-commits]

meadori added inline comments.


================
Comment at: ELF/LinkerScript.cpp:419-422
+      error("section '" + CurOutSec->Name + "' will not fit in region '" +
+            CurMemRegion->Name + "'");
+      error("region '" + CurMemRegion->Name + "' overflowed by " +
+            Twine(OverflowAmt) + " bytes");
----------------
ruiu wrote:
> You don't want to call `error` twice for one error. You can print out a multi-line error just by adding "\n" in middle of a message.
I originally did that, but the formatting looks bad:

```
./bin/ld.lld: error: section '.data' will not fit in region 'ram'
region 'ram' overflowed by 3084 bytes
```

Personally I think the following is easier to understand:

```
./bin/ld.lld: error: section '.data' will not fit in region 'ram'
./bin/ld.lld: error: region 'ram' overflowed by 3084 bytes
```


================
Comment at: ELF/LinkerScript.cpp:541-542
+
+  if (!Opt.MemoryRegions.size())
+    return nullptr;
+
----------------
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.


================
Comment at: ELF/LinkerScript.cpp:549
+    if ((MR.Flags & Sec->Flags) != 0 && (MR.NotFlags & Sec->Flags) == 0) {
+      MatchingMemRegion = &MR;
+      break;
----------------
ruiu wrote:
> Return `&MR` here and remove `MatchingMemRegion`
Will fix.


================
Comment at: ELF/LinkerScript.cpp:594
                 [this](std::unique_ptr<BaseCommand> &B) { process(*B.get()); });
+  CurMemRegion = nullptr;
 }
----------------
ruiu wrote:
> Why do you have to clear this?
I don't, that was a left over from a previous revision.  Thank you for catching it.


================
Comment at: ELF/LinkerScript.cpp:2047
+      Opt.MemoryRegions[Name] =
+          MemoryRegion(Name, Origin, Length, Flags, NotFlags);
+  }
----------------
ruiu wrote:
> You can change this to
> 
>   Opt.MemoryRegions[Name] = {Name, Origin, Length, Flags, NotFlags};
> 
> can't you?
I can't b/c of reasons cited in previous comments.


================
Comment at: ELF/LinkerScript.cpp:2053
+// into a section flag.
+uint32_t ScriptParser::memoryAttributeToFlag(char Attr) {
+  if (tolower(Attr) == 'w')
----------------
ruiu wrote:
> You can make this a static non-member function (which is generally preferred because it makes clear that the function does not depend on any class.)
I can't call `setError` if I do that.  I think it is better to just inline that code back into `readMemoryAttributes`.


================
Comment at: ELF/LinkerScript.h:196
+struct MemoryRegion {
+  MemoryRegion() : Origin(0), Length(0), Index(0), Flags(0), NotFlags(0) {}
+  MemoryRegion(StringRef N, uint64_t O, uint64_t L, uint32_t F, uint32_t NF)
----------------
ruiu wrote:
> Instead of defining this constructor, can you set default values by adding `= 0` to all these fields?
I can't b/c of the error I describe in the following comment.  Since I can't remove the second constructor I have to have a default one (otherwise the compiler doesn't provide one for me).


================
Comment at: ELF/LinkerScript.h:197-198
+  MemoryRegion() : Origin(0), Length(0), Index(0), Flags(0), NotFlags(0) {}
+  MemoryRegion(StringRef N, uint64_t O, uint64_t L, uint32_t F, uint32_t NF)
+      : Name(N), Origin(O), Length(L), Index(O), Flags(F), NotFlags(NF) {}
+
----------------
ruiu wrote:
> I think you can remove this constructor if you initialize a struct using `{}` as I mentioned below.
That doesn't work. I get errors like:

```
error: no match for ‘operator=’ (operand types are ‘lld::elf::MemoryRegion’ and ‘<brace-enclosed initializer list>’)
```


https://reviews.llvm.org/D28911