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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo accepted this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

The change itself LGTM



================
Comment at: llvm/test/tools/llvm-objdump/COFF/long-section-name.test:19
+template = bytes(sys.argv[2], 'utf-8')
+replacement = b'/4\0abcde'
+
----------------
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.


================
Comment at: llvm/test/tools/llvm-objdump/COFF/long-section-name.test:26
+  pos = data.find(template)
+  outp.write(data[:pos])
+  outp.write(replacement)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127369