This is an archive of the discontinued LLVM Phabricator instance.

[MC] Fix memory leak when allocating MCInst with bump allocator
AcceptedPublic

Authored by hgreving on Jul 29 2020, 4:53 PM.

Details

Summary

Adds the function createMCInst() to MCContext that creates a MCInst using
a typed bump alloctor.

MCInst contains a SmallVector<MCOperand, 8>. The SmallVector is POD only
for <= 8 operands. The default untyped bump pointer allocator of MCContext
does not delete the MCInst, so if the SmallVector grows, it's a leak.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46900.

Diff Detail

Event Timeline

hgreving created this revision.Jul 29 2020, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2020, 4:53 PM
hgreving requested review of this revision.Jul 29 2020, 4:53 PM
hgreving edited the summary of this revision. (Show Details)Jul 29 2020, 5:01 PM
hgreving edited the summary of this revision. (Show Details)Jul 29 2020, 5:11 PM
hgreving added reviewers: kariddi, majnemer, bcain, MaskRay.

I have added some people w/o knowing them based on contributions to Hexagon and MC. Please accept or reassign the review if possible.

hgreving edited the summary of this revision. (Show Details)Jul 29 2020, 5:52 PM
kariddi accepted this revision.Jul 29 2020, 6:30 PM
kariddi added inline comments.
llvm/lib/MC/MCContext.cpp
135

There is usually an extra space between the new and the "(" when a placement-like new is used in LLVM.

This revision is now accepted and ready to land.Jul 29 2020, 6:30 PM

Add createMCInst() using a typed MCInst bump allocator.

For titles most patches don't add a full stop. The important fact (it fixes a memory leak) should be mentioned.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46900.

Add a little detail beside the bug reference about the memory leak.

llvm/include/llvm/MC/MCContext.h
384

I think @{ is used very rarely for new code. You may delete it which does not add any documentation value.

MaskRay accepted this revision.Jul 29 2020, 9:44 PM
hgreving updated this revision to Diff 281972.Jul 30 2020, 10:24 AM
hgreving marked 2 inline comments as done.
hgreving retitled this revision from [MC] Add createMCInst() using a typed MCInst bump allocator. to [MC] Fix memory leak when allocating MCInst with bump allocator.
hgreving edited the summary of this revision. (Show Details)

Done.

hgreving edited the summary of this revision. (Show Details)Jul 30 2020, 10:26 AM
MaskRay accepted this revision.Jul 30 2020, 10:32 AM

LGTM.

Tending to push this rel. small fix w/o an explicit Hexagon sign-off. Objections?

bcain added a comment.Sep 12 2020, 6:39 PM

Tending to push this rel. small fix w/o an explicit Hexagon sign-off. Objections?

Sorry for not giving this attention when it was merged -- LGTM.