Page MenuHomePhabricator

Avoid double deletion in Clang driver.
ClosedPublic

Authored by sepavloff on Dec 11 2015, 6:24 AM.

Details

Summary

Llvm module object is shared between CodeGenerator and BackendConsumer,
in both classes it is stored as std::unique_ptr, which is not a good
design solution and can cause double deletion error. Usually it does
not occur because in BackendConsumer::HandleTranslationUnit the
ownership of CodeGenerator over the module is taken away. If however
this method is not called, the module is deleted twice and compiler crashes.

As the module owned by BackendConsumer is always the same as CodeGenerator
has, local copy of llvm module can be removed from BackendGenerator.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff updated this revision to Diff 42519.Dec 11 2015, 6:24 AM
sepavloff retitled this revision from to Avoid double deletion in Clang driver..
sepavloff updated this object.
sepavloff added a subscriber: cfe-commits.

Any feedback?

Thanks,
--Serge

Can somebody have a look at this fix?
Thank you!

--Serge

sepavloff updated this revision to Diff 46575.Feb 1 2016, 1:31 PM

Added regression test.

Any feedback?

Thanks,
--Serge

CodeGen action defined in the unit test does not call HandleTranslationUnit,
so the hack implemented in BackendConsumer::HandleTranslationUnit does
not run and clang crashes. With the fix no crash occurs.

Thanks,
--Serge

It certainly makes sense to redirect the module request to its owner instead of duplicating it in a local copy.

There may be a change in behaviour here, pre-patch CodeGenAction::EndSourceFileAction() would take the BackendConsumer Module, keeping CodeGen Module intact whereas post-patch it takes the CodeGen Module.
How does this work out?

It certainly makes sense to redirect the module request to its owner instead of duplicating it in a local copy.

It is not even a duplication, it is simultaneous sharing by two unique_ptr's.

There may be a change in behaviour here, pre-patch CodeGenAction::EndSourceFileAction() would take the BackendConsumer Module, keeping CodeGen Module intact whereas post-patch it takes the CodeGen Module.
How does this work out?

It should not be a problem. CodeGenAction::EndSourceFileAction() is called when code generation is finished, so taking ownership from GodeGen looks safe. Anyway, module must not be owned by two unique_ptr's.

There are at least 3 unique_ptr's that owns the same module object:

  • CodeGenAction::TheModule
  • BackendConsumer::TheModule
  • CodeGeneratorImpl::M

It looks like CodeGenAction::TheModule does not cause any problem, it takes ownership according to usual unique_ptr semantics. But the last two variables owns the same object simultaneously, which violates semantics of unique_ptr. The patch fixes ownership by allowing only CodeGeneratorImpl::M to be an owner.

yaron.keren accepted this revision.Feb 17 2016, 8:23 AM
yaron.keren added a reviewer: yaron.keren.

Would be nice to have CodeGenAction::TheModule redirect to CodeGeneratorImpl::M if possible, but that's for another patch. LGTM.

This revision is now accepted and ready to land.Feb 17 2016, 8:23 AM
This revision was automatically updated to reflect the committed changes.