[PATCH] D127369: [Object][COFF] Fix section name parsing error when the name field is not null-padded

Thu Jun 9 10:46:08 PDT 2022

pzheng marked 2 inline comments as done.
pzheng added inline comments.

================
Comment at: llvm/test/tools/llvm-objdump/COFF/long-section-name.test:19
+template = bytes(sys.argv[2], 'utf-8')
+replacement = b'/4\0abcde'
+
----------------
mstorsjo wrote:
> As the replacement is hardcoded to contain `/4` here, I think it feels slightly inconsistent to take the pattern `/4` as a parameter. This test wouldn't really work with any other parameter than `/4` (even if the symbol would constain a different offset than `4`, then the replacement pattern would need to have the same offset too.
I agree, it's better to just hard code /4 into the test.

================
Comment at: llvm/test/tools/llvm-objdump/COFF/long-section-name.test:26
+  pos = data.find(template)
+  outp.write(data[:pos])
+  outp.write(replacement)
----------------
mstorsjo wrote:
> I was wondering about whether the test is slightly brittle - if the pattern isn't found at all, I presume that this process would fail somehow (if `pos` returns as -1). I'd just want to avoid that the test silently keeps passing if the offset turns out different for some reason.
> 
> Maybe it'd be safest with a check like `if pos == -1: exit(1)` to make it clearer?
Makes sense to me too. Thanks for the suggestion!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127369