[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 7 inline comments as done.
abrachet added a comment.

In D64281#1638215 <https://reviews.llvm.org/D64281#1638215>, @rupprecht wrote:

> FYI, you'll have to rebase this whole patch series after rL368826 <https://reviews.llvm.org/rL368826> due to `Section::getName()` changing signature


I was safe for this patch it seems but not others, thanks for the heads up.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:113
+    NewType &makeMutable(uint64_t Index, Args &&... Arguments) {
+      assert(Index < Mappings.size() && "Out of bounds");
+      if (Mappings[Index].Type == MappingType::New)
----------------
rupprecht wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > MaskRay wrote:
> > > > I think such `Out of bounds` assertions are probably not very necessary. They will certainly segfault if the user does have out-of-bounds accesses.
> > > Won't that entirely depend on how the memory is laid out, compiler-generated checks etc? A segmentation fault hardly provides any useful information, whereas at least the assertion shows what's going wrong in builds with assertions enabled...
> > I think if the user of this library makes an out-of-bounds access, the program will very quickly segfault with a clear stack trace (symbolization can happen with a symbol table, even if debug info is absent) that the problem is related to the use of MutableELFObject.
> > 
> > There are lots of ways to detect errors: -D_GLIBCXX_DEBUG / asan / ...
> > 
> > We can add some assert() in some tricky places (they also serve as documentation of some invariants) but here I think it is probably not very necessary.
> > I think if the user of this library makes an out-of-bounds access, the program will very quickly segfault with a clear stack trace
> Having dealt with many prod crashes in the past, I strongly disagree. C++ optimizations can often make the crash happen far away from the actual bug.
> 
> I think it's much more common to run tools in debug mode than in asan, so it's useful to have these.
I haven't removed the assertions in this diff. I think worst case it looks ugly and I agree that they aren't super necessary but they have helped me track bugs down before so I think the benefit outweighs the cost.


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

https://reviews.llvm.org/D64281