[PATCH] D50494: [LLD] Reserve memory in implicitly set LMARegions

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:824
+  if (Ctx->LMARegion && !areFlagsCompatible(Ctx->LMARegion, Sec))
+    warn("requested to locate " + Sec->Name + " into lma region with"
+         " incompatible flags");
----------------
kschwarz wrote:
> grimar wrote:
> > I think:
> > 1) LMA perhaps should be uppercase in the error message.
> > 2) Would be nice to include memory region name to this warning.
> After a second thought, I'm not sure if this warning really makes sense.
> Initialized data will be copied to RAM at runtime, but has to be loaded to read-only memory. In that case, the flags will obviously not be compatible, and the warning will be confusing. 
> 
I think its ok to remove it. Patch already do much, we can add/polish the additional warnings/error messages in followups.


================
Comment at: ELF/LinkerScript.cpp:826
   else if (Sec->AddrExpr)
-    setDot(Sec->AddrExpr, Sec->Location, false);
+    moveTo(Sec->AddrExpr().getValue());
 
----------------
Seems it is needed for `vma.test` only. What do you think about splitting this change to separate followup patch (with vma.test)?
I would not introduce `moveTo` in this patch because this seems to be more or less isolated/independent change and
since we already have `setDot`, I am not sure it is good to introduce one more method that moves the `Dot`. I would probably
try to inline it, but first of all, I think it can be discussed/reviewed separately.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50494