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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

This looks good to me. Few comments below.



================
Comment at: ELF/LinkerScript.cpp:761
 
+static MemoryRegion *findContainingSection(uint64_t addr) {
+  for (auto &Pair : Script->MemoryRegions) {
----------------
`addr` -> `Addr`

It feels that `findContainingSection` is a wrong name. You are finding and returning the memory region.
I would suggest `findRegion` or something alike.


================
Comment at: ELF/LinkerScript.cpp:783
+  // LMARegion in the context
+  if (Sec->LMARegion) {
+    Ctx->LMARegion = Sec->LMARegion;
----------------
According to LLD code style, you do not need adding braces for a single line. (Here and below).


================
Comment at: test/ELF/linkerscript/Inputs/at10.s:1
+.section .sec1,"aw", at progbits
+.quad   1
----------------
I would reuse `at8.s` input. It contains the same sections.
We sometimes reusing the inputs from other tests when it is convenient, I believe it is the case here.


================
Comment at: test/ELF/linkerscript/Inputs/at9.s:1
+.section .sec1,"aw", at progbits
+.quad   1
----------------
The same.


================
Comment at: test/ELF/linkerscript/at10.test:20
+# Otherwise, .sec3 will show a huge PhysAddr because of
+# unsigned underflow in the calculation of the LMAOffset
+
----------------
I think this comment (and the comment in the test below) has the following "issue":
You are referring to the `PhysAddr` and `LMAOffset`, which are the details of the implementation
and perhaps should not be mentioned here as might change in theory.

What I would suggest is to avoid using names of variables in test cases and use just regular words
to describe the problem.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50494