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

[Roger Kim via Phabricator via llvm-commits]

Roger added a comment.

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



================
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())
----------------
Why is there a copy of `*off` for errors? Could we not use `*off` in the case of errors?


================
Comment at: lld/MachO/EhFrame.cpp:62
+uint64_t EhReader::readPointer(size_t *off) const {
+  if (*off + wordSize == data.size())
+    failOn(*off, "unexpected end of CIE/FDE");
----------------
In the other functions, you use a `>` comparison. Why do you use an equality check here?


================
Comment at: lld/MachO/EhFrame.cpp:77-78
+StringRef EhReader::readString(size_t *off) const {
+  auto *c = reinterpret_cast<const char *>(data.data() + *off);
+  size_t len = strnlen(c, data.size());
+  if (len == data.size())
----------------
Is there no need to check that `*off + 1 < data.size()` (+ 1 to have room for the null symbol)? I think we could potentially segfault if *off is too large? 


================
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)");
----------------
I could go either way, but what do you think about a comment like this just to make things obvious to the reader?


================
Comment at: lld/MachO/InputFiles.cpp:1303-1305
+        funcAddrRelocIt = it++;
+      else if (lsdaAddrOpt && it->offset == lsdaAddrOff)
+        lsdaAddrRelocIt = it++;
----------------
I'm a bit surprised why we want to use `it++` instead of just `it` given that we're in a for loop that already increments `it` for us. Could we have a comment explaining this behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123435