[PATCH] D123435: [lld-macho] Initial support for EH Frames

[Jez Ng via Phabricator via llvm-commits]

int3 marked 5 inline comments as done.
int3 added a comment.

> I noticed that the tests in eh-frame.s only contain failing cases. Are there already success cases tested elsewhere?

There's `eh-frame.s` and `invalid/eh-frame.s`. I'm guessing you only looked at the latter :)



================
Comment at: lld/MachO/EhFrame.cpp:22
+uint64_t EhReader::readLength(size_t *off) const {
+  const size_t errOff = *off;
+  if (*off + 4 > data.size())
----------------
Roger wrote:
> Why is there a copy of `*off` for errors? Could we not use `*off` in the case of errors?
I thought it'd be a bit clearer to have the error message point at the start of a mis-encoded length field, rather than in the middle of the value.

Also for line 35 below, when saying that a CIE/FDE extends past the end, I think it would make sense to point to the start of the CIE/FDE, rather than in the middle


================
Comment at: lld/MachO/EhFrame.cpp:79
+  size_t len = strnlen(c, data.size());
+  if (len == data.size())
+    failOn(*off, "corrupted CIE (failed to read string)");
----------------
Roger wrote:
> I could go either way, but what do you think about a comment like this just to make things obvious to the reader?
I went with something a bit more terse. Note that `man strnlen` already says

> The strnlen() function returns
> either the same result as strlen() or maxlen, whichever is smaller.

but -- I realized belatedly that this entire function is wrong. I was not checking the right `maxlen`... fixing + adding test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123435