Page MenuHomePhabricator

[DSE] Check post-dominance for malloc+memset->calloc transform.
ClosedPublic

Authored by asbirlea on Aug 20 2021, 2:06 PM.

Diff Detail

Event Timeline

asbirlea created this revision.Aug 20 2021, 2:06 PM
asbirlea requested review of this revision.Aug 20 2021, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 2:06 PM
xbolva00 added a subscriber: xbolva00.EditedAug 20 2021, 2:22 PM

Also some notes from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83022

implementing a check to see if the only condition between the malloc and memset is 'ptr != 0'.

+1. This is probably what we really want to make everybody happy.

llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
415

I would rather see better fix, so this motivating case still works (allow 'ptr compared with NULL'). Othewise I see no value from this optimization at all and it should be just removed from DSE.

I am somewhat scared that people talking about this pattern

memset(malloc(size), 0, size)

not suprised that why there are so many nasty bugs/exploits in C codebases.

asbirlea added inline comments.Aug 20 2021, 2:50 PM
llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
415

I would rather see better fix, so this motivating case still works (allow 'ptr compared with NULL'). Othewise I see no value from this optimization at all and it should be just removed from DSE.

Sure, revert is a viable option while looking to add the 'ptr compared with NULL' check, as that's not so straightforward.

xbolva00 added inline comments.Aug 20 2021, 3:18 PM
llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
415

But probably we should just take your patch, to avoid regression as SLC's transformation was removed as DSE-based one was implemented.

yurai007 added inline comments.Aug 21 2021, 2:56 AM
llvm/test/Transforms/DeadStoreElimination/noop-stores.ll
415

'ptr compared with NULL' check, as that's not so straightforward.

Yes, it requires some dance with matching like here (old patch doing transformation in SLC): https://reviews.llvm.org/D101176

alexfh accepted this revision.Aug 23 2021, 9:53 AM

Looks good to me!

This revision is now accepted and ready to land.Aug 23 2021, 9:53 AM

I think, it's the right first step, and if there's a need to make the transformation support more use cases like checking the result of malloc for null, that can be added as a separate step.