[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:
> > 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"?
> That works.  It looks like this now:
> 
> ```
> error: section '.data' will not fit in region 'ram': overflowed by 3084 bytes
> ```
Looks nice.



================
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:
> > 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}
> I accounted for that.  It seems that in-class field initialization and braced initializer lists are not compatible:
> 
> ```
> $ c++ --version
> c++ (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
> Copyright (C) 2013 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> meadori at build1-trusty-cs:/scratch/meadori/llvm-mainline/obj/llvm$ cat -b test.cpp
>      1  struct T {
>      2    unsigned x = 0;
>      3  };
>      4  int main(void) {
>      5    T t = {1};
>      6    return 0;
>      7  }
> meadori at build1-trusty-cs:/scratch/meadori/llvm-mainline/obj/llvm$ c++ -std=c++11 test.cpp
> test.cpp: In function ‘int main()’:
> test.cpp:5:11: error: could not convert ‘{1}’ from ‘<brace-enclosed initializer list>’ to ‘T’
>    T t = {1};
>            ^
> ```
> 
> If I remove the in-class initializers, it works.
Sure. Then I think you can remove the C++11-style intializers. I think we don't need it.


https://reviews.llvm.org/D28911