[PATCH] D98668: Support !heapallocsite attachments in StripDebugInfo().

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a subscriber: akhuang.
vsk added inline comments.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:350
+      if (I.hasMetadataOtherThanDebugLoc())
+        I.setMetadata("heapallocsite", nullptr);
     }
----------------
dexonsmith wrote:
> vsk wrote:
> > Is it possible to reach this when the instruction has no heapallocsite metadata?
> > 
> > If so, we might want a narrower change, like:
> > ```
> > if (auto *MD = I.getMetadata("heapallocsite"))
> >   I.setMetadata("heapallocsite", nullptr);
> > ```
> > 
> > Or we might want to guarantee that we drop non-debug metadata, like:
> > ```
> > I.dropUnknownNonDebugMetadata();
> > ```
> > 
> > Ditto for D98667.
> > Is it possible to reach this when the instruction has no heapallocsite metadata?
> 
> It is possible, but the code as written is correct. `nullptr` here effectively means "delete if exists".
> 
> We could (maybe should?) drop the `if` entirely and always do:
> ```
> I.setMetadata("healallocsite", nullptr);
> ```
> since Instruction::setMetadata has an early return for exactly the checked condition.
> 
> > Or we might want to guarantee that we drop non-debug metadata, like:
> 
> Dropping non-debug metadata when stripping debug info sounds dangerous to me, since debug info is not supposed to have an impact on optimizations. (I was worried at first when I saw this patch because `heapallocsite` sounds like an optimization hint -- in which case I was going to suggest replacing the metadata with a remapped / equivalent version -- but `git grep` told me it's only used for debug info. Probably it should be renamed `dbg.heapallocsite` or something to make it clear that it shouldn't be used for anything else...)
> 
Dropping the check for I.hasMetadataOtherThanDebugLoc() and renaming "heapallocsite" to "dbg.heapallocsite" (or similar) both sound good to me (cc'ing @akhuang).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98668