[PATCH] D19272: [ELF] - linkerscript AT keyword (in output section description) implemented.

[George Rimar via llvm-commits]

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