[PATCH] D50494: [LLD] Reserve memory in implicitly set LMARegions
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 9 01:05:51 PDT 2018
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
More information about the llvm-commits
mailing list