[PATCH] D19272: [ELF] - linkerscript AT keyword (in output section description) implemented.
George Rimar via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 20 03:17:48 PDT 2016
grimar added inline comments.
================
Comment at: ELF/LinkerScript.cpp:170
@@ -169,1 +169,3 @@
+uint64_t LinkerScript::getPA(uint64_t VA, StringRef Name) {
+ auto I = PhysAddrExpr.find(Name);
----------------
ruiu wrote:
> Ah, so PA stands for Physical Address, but is it a common name? At least it doesn't feel "physical" to me. Why don't you name this getAddr?
>
> Also I'd rename Name -> SectionName.
I took "physical address" term from description of p_paddr field:
https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-83432/index.html
I did not name getAddr() because it seems confusing to me, when I hear "address" I usually think about VMA, since LMA is not
really common.
But I think we can call it getLma(), as LMA is a term used for AT expressions:
https://www.sourceware.org/binutils/docs-2.12/ld.info/Output-Section-LMA.html
================
Comment at: ELF/LinkerScript.cpp:232
@@ -224,2 +231,3 @@
void readSectionPatterns(StringRef OutSec, bool Keep);
+ void readCommandAt(StringRef OutSec);
----------------
ruiu wrote:
> readAt for consistency.
Done.
================
Comment at: ELF/LinkerScript.cpp:434
@@ +433,3 @@
+ if (Expr.empty())
+ error("empty AT command expression");
+}
----------------
ruiu wrote:
> Use setError.
Done.
================
Comment at: ELF/LinkerScript.cpp:442-444
@@ -419,1 +441,5 @@
expect(":");
+ StringRef Tok = peek();
+ if (Tok == "AT")
+ readCommandAt(OutSec);
+
----------------
ruiu wrote:
> No need to reuse a variable.
>
> if (peek() == "AT")
> readAt(OutSec);
Done.
================
Comment at: ELF/LinkerScript.h:91
@@ +90,3 @@
+ // used in AT command implementation.
+ llvm::StringMap<std::vector<StringRef>> PhysAddrExpr;
+
----------------
ruiu wrote:
> StringMap copies keys. Do you want to use DenseMap instead?
Yes, I know it do. I see no problem here. StringMap can be a bit faster for lookup, but this place should not be
bottleneck though, so I think there are no reasons to use it or not to do that here,
because amount of keys should be little either.
I used it mostly to be consistent with Filler field. May be we should combine them into one variable ?
```
struct OutputSectionData {
std::vector<uint8_t> Filler;
std::vector<StringRef> PhysAddrExpr;
};
llvm::StringMap<OutputSectionData> OutSections;
```
================
Comment at: ELF/Writer.cpp:1464-1466
@@ -1463,3 +1463,5 @@
- // If flags changed then we want new load segment.
+ bool HasPA = Script->hasPA(Sec->getName());
+ // If AT linker script command was used for section or if flags were
+ // changed, then we want new load segment.
uintX_t NewFlags = toPhdrFlags(Sec->getFlags());
----------------
ruiu wrote:
> Why we want to create a new load segment for sections with AT commands?
Section header has only one address field sh_addr which is VMA:
https://docs.oracle.com/cd/E19455-01/806-3773/elf-2/index.html
So there is not way to set LMA for section, only for load segment itself I believe.
And that means that for section that explicitly requested LMA new load has
to be created.
Both gold and bfd do the same.
AT commands changes p_paddr of PT_LOAD header in binutils:
https://sourceware.org/ml/binutils/2011-06/msg00262.html
http://reviews.llvm.org/D19272
More information about the llvm-commits
mailing list