[PATCH] D71647: [llvm-objcopy][MachO] Handle relocation entries where r_extern is 0

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked an inline comment as done.
seiya added a comment.

BTW, I noticed that we need a same fix for `n_sect` field in nlist (in a separate patch).



================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.h:93
   // corresponding content inside the binary.
-  std::vector<Section> Sections;
+  std::vector<std::unique_ptr<Section>> Sections;
 
----------------
jhenderson wrote:
> seiya wrote:
> > jhenderson wrote:
> > > Is this really necessary? As far as I can tell, the only place it's actually important is where you create a vector of section pointers, but you can do that without needing these to be unique_ptrs. I've commented above.
> > I think this should be unique pointers. Pointers to vector elements could be invalidated by --add-section/--remove-section operations.
> > 
> Okay, that's a good point, but why wasn't it an issue already?
> 
> Assuming that it's actually important, I think moving the unique_ptr changes into a separate patch would make it more obvious what's actually important for this functional change.
> Okay, that's a good point, but why wasn't it an issue already?

I think this bug has not been discovered because modifying relocatable objects that enable this feature (r_extern = 0) is a rare case.

> Assuming that it's actually important, I think moving the unique_ptr changes into a separate patch would make it more obvious what's actually important for this functional change.

Moving unique_ptr changes into a separate parch sounds good to me. I'll submit it.


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

https://reviews.llvm.org/D71647