[PATCH] MC: Support larger COFF string tables

[Richard]

================
Comment at: lib/MC/WinCOFFObjectWriter.cpp:501-503
@@ +500,5 @@
+
+    if (StringTableEntry <= 999999) {
+      std::sprintf(S.Header.Name, "/%d", unsigned(StringTableEntry));
+    } else if (StringTableEntry <= 9999999) {
+      // With seven digits, we have to skip the terminating null. Because
----------------
These two constants are so nearly identical I had to read this really carefully to see that you didn't have the same condition in both if statements.

How about:

  const int MAX_6_DECIMAL_DIGITS = 999999;
  const int MAX_7_DECIMAL_DIGITS = 9999999;

  if (StringTableEntry <= MAX_6_DECIMAL_DIGITS) {
  // ...
  } else if (StringTableEntry <= MAX_7_DECIMAL_DIGITS) {
  // ...
  }

I'm not fussy about the name, but these two nearly-identical digit-strings are a recipe for confusion IMO.

================
Comment at: lib/MC/WinCOFFObjectWriter.cpp:509
@@ +508,3 @@
+      std::memcpy(S.Header.Name, buffer, 8);
+    } else if (StringTableEntry <= 0xFFFFFFFFF) {
+      // Starting with 10,000,000, offsets are encoded as base64.
----------------
A symbolic name for this magic hex number would also increase understanding.

================
Comment at: lib/MC/WinCOFFObjectWriter.cpp:476
@@ +475,3 @@
+// Buffer must be at least 8 bytes large. No terminating null appended.
+static void encodeBase64StringEntry(char* Buffer, uint64_t Value) {
+  assert(Value > 9999999 && Value <= 0xFFFFFFFFF &&
----------------
This seems like something that should be trivial to unit test as it is not directly coupled to anything else and its inputs and outputs are parameters.

================
Comment at: lib/Object/COFFObjectFile.cpp:57
@@ +56,3 @@
+// prefixed slashes.
+static bool decodeBase64StringEntry(StringRef Str, uint32_t &Result) {
+  assert(Str.size() <= 6 && "String too long, possible overflow.");
----------------
This seems like something that should be trivial to unit test as it is not directly coupled to anything else and its inputs and outputs are parameters.

It seems odd that this function returns true on failure and false on success.

When reading the code aloud it sounds as "if decode base64 string entry then failed.", which seems backwards.  I'd rather see true indicating success and false indicating failure.  This does mean that you have to reverse the test in your if statements later on, but when you do they read more obviously: "if not decode base64 string entry then failed."


http://llvm-reviews.chandlerc.com/D667

BRANCH
  arc/667-coff-string-table-v3

ARCANIST PROJECT
  llvm