[PATCH] D146534: [llvm-nm] Print EC symbol map.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks to me like you should have test cases for your EC Map validation.



================
Comment at: llvm/lib/Object/Archive.cpp:952-955
+    // ARM64EC-aware libraries contain an additional special member with
+    // EC symbol map after string table. Its format is similar to regular
+    // symbol map, except it doesn't contain member offsets. Its
+    // indexes refer to member offsets from the regular symbol table instead.
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:1193
+        SymbolTable.size() < sizeof(uint32_t))
+      return malformedError("invalid EC symbols size");
+
----------------
The LLVM coding standards state you should give additional context in error messages that would help diagnose the issue better (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages) In this case, you could say "invalid EC symbol table size (<ECSymTable.size()>)".


================
Comment at: llvm/lib/Object/Archive.cpp:1197
+    if (ECSymbolTable.size() < sizeof(uint32_t) + Count * sizeof(uint16_t))
+      return malformedError("invalid EC symbols size");
+
----------------
As above, more context could be included, e.g. "invalid EC symbols size. Size was <ECSymbolTable.size()>, but expected <sizeof(uint32_t) + Count * sizeof(uint16_t))>" (obviously expanding the bits in "<>").


================
Comment at: llvm/lib/Object/Archive.cpp:1201
+    const char *Indexes = ECSymbolTable.begin() + sizeof(uint32_t);
+    size_t StringIndex = sizeof(uint32_t) + Count * sizeof(uint16_t);
+
----------------
If you moved this variable definition before the previous error check, you'd be able to reuse it in that error check, rather than duplicating the calculation.


================
Comment at: llvm/lib/Object/Archive.cpp:1206
+      if (!Index || Index > MemberCount)
+        return malformedError("invalid EC symbol index");
+      StringIndex = ECSymbolTable.find('\0', StringIndex);
----------------
As above, include the index and member count in the message for additional context.


================
Comment at: llvm/lib/Object/Archive.cpp:1209
+      if (StringIndex == StringRef::npos)
+        return malformedError("malformed EC symbols names");
+      ++StringIndex;
----------------
Perhaps: "malformed EC symbol names: not null-terminated" to clarify to the user why it appears malformed? Also, note "symbols" -> "symbol" regardless of the additional context.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146534/new/

https://reviews.llvm.org/D146534