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

[Rui Ueyama via llvm-commits]

ruiu 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);
----------------
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.

================
Comment at: ELF/LinkerScript.cpp:232
@@ -224,2 +231,3 @@
   void readSectionPatterns(StringRef OutSec, bool Keep);
+  void readCommandAt(StringRef OutSec);
 
----------------
readAt for consistency.

================
Comment at: ELF/LinkerScript.cpp:434
@@ +433,3 @@
+  if (Expr.empty())
+    error("empty AT command expression");
+}
----------------
Use setError.

================
Comment at: ELF/LinkerScript.cpp:442-444
@@ -419,1 +441,5 @@
   expect(":");
+  StringRef Tok = peek();
+  if (Tok == "AT")
+    readCommandAt(OutSec);
+
----------------
No need to reuse a variable.

  if (peek() == "AT")
   readAt(OutSec);

================
Comment at: ELF/LinkerScript.h:71
@@ -70,1 +70,3 @@
   int compareSections(StringRef A, StringRef B);
+  bool hasPA(StringRef Name) { return PhysAddrExpr.count(Name) > 0; }
+  uint64_t getPA(uint64_t VA, StringRef Name);
----------------
What does PA stand for?

================
Comment at: ELF/LinkerScript.h:91
@@ +90,3 @@
+  // used in AT command implementation.
+  llvm::StringMap<std::vector<StringRef>> PhysAddrExpr;
+
----------------
StringMap copies keys. Do you want to use DenseMap instead?

================
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());
----------------
Why we want to create a new load segment for sections with AT commands?


http://reviews.llvm.org/D19272