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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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");
----------------
meadori wrote:
> 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
> ```
Semantically I want to emit one `error:` for each error. I don't think this needs to be in two lines -- how about concatenating these messages with ": " instead of "\n"?


================
Comment at: ELF/LinkerScript.cpp:2023
+    if (!(consume("ORIGIN") || consume("org") || consume("o")))
+      setError("expected one of: ORIGIN, org, or o");
+    expect("=");
----------------
You want to return from the function when you find an error in this function to prevent cascading error messages.


================
Comment at: ELF/LinkerScript.cpp:2072
+  bool Invert = false;
+  for (auto C : next()) {
+    if (C == '!')
----------------
s/auto/char/


================
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) {}
+
----------------
meadori wrote:
> 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>’)
> ```
Sorry, the number of initializers did not match with the number of members. Does this work?

  {Name, Origin, Length, Origin, Flags, NotFlags}


https://reviews.llvm.org/D28911