[PATCH] D118692: [llvm-objcopy][COFF] Fix section name encoding

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/section-name-encoding.s:1-2
+// Check that COFF section names of sections added by llvm-objcopy are properly
+// encoded.
+//
----------------
llvm-objcopy generally uses `#` and `##` in lit test files, the former for test directives, like RUN lines, and FileCheck patterns, the latter for pure comments.

(If llvm-mc doesn't like `#` as a comment marker, then I'd use `////` still for true comments, to distinguish them from test directives).


================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:126
+// Buffer must be at least 8 bytes large. No terminating null appended.
+static void encodeBase64StringEntry(char *Buffer, uint64_t Value) {
+  assert(Value > Max7DecimalOffset && Value <= MaxBase64Offset &&
----------------
In terms of avoiding duplicated code, I think you should, in a separate prerequisite patch, move the duplicate code from `WinCOFFObjectWriter.cpp` to somewhere like the `BinaryFormat` or `Object` library. It's not immediately clear to me which is more appropriate though.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:172
+        // it adds a null termination character.
+        snprintf(Str, sizeof(S.Header.Name) + 1, "/%d", (int)Offset);
+      } else if (Offset <= MaxBase64Offset) {
----------------
I'd use `static_cast` rather than a C-style cast, whilst you're changing this line.


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

https://reviews.llvm.org/D118692