This is an archive of the discontinued LLVM Phabricator instance.

Support !heapallocsite attachments in StripDebugInfo().
ClosedPublic

Authored by aprantl on Mar 15 2021, 4:26 PM.

Details

Summary

They point into the DIType type system, so they need to be stripped as well.

rdar://75341300

Diff Detail

Event Timeline

aprantl created this revision.Mar 15 2021, 4:26 PM
aprantl requested review of this revision.Mar 15 2021, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 4:26 PM
aprantl retitled this revision from Support !heapallocsite attachments in StripTableDebugInfo(). to Support !heapallocsite attachments in StripDebugInfo(). .Mar 15 2021, 4:30 PM
This revision is now accepted and ready to land.Mar 15 2021, 5:15 PM
vsk added inline comments.Mar 16 2021, 9:59 AM
llvm/lib/IR/DebugInfo.cpp
350

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.

This revision was landed with ongoing or failed builds.Mar 16 2021, 10:05 AM
This revision was automatically updated to reflect the committed changes.
dexonsmith added inline comments.Mar 16 2021, 12:32 PM
llvm/lib/IR/DebugInfo.cpp
350

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...)

vsk added a subscriber: akhuang.Mar 16 2021, 12:38 PM
vsk added inline comments.
llvm/lib/IR/DebugInfo.cpp
350

Dropping the check for I.hasMetadataOtherThanDebugLoc() and renaming "heapallocsite" to "dbg.heapallocsite" (or similar) both sound good to me (cc'ing @akhuang).