r312580 - Fix memory leak after r312467. The ModuleMap is the owner of the global module object until it's reparented under a real module.

[David Blaikie via cfe-commits]

On Tue, Sep 5, 2017 at 2:47 PM Richard Smith via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: rsmith
> Date: Tue Sep  5 14:46:22 2017
> New Revision: 312580
>
> URL: http://llvm.org/viewvc/llvm-project?rev=312580&view=rev
> Log:
> Fix memory leak after r312467. The ModuleMap is the owner of the global
> module object until it's reparented under a real module.
>
> Modified:
>     cfe/trunk/include/clang/Lex/ModuleMap.h
>     cfe/trunk/lib/Lex/ModuleMap.cpp
>
> Modified: cfe/trunk/include/clang/Lex/ModuleMap.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/ModuleMap.h?rev=312580&r1=312579&r2=312580&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Lex/ModuleMap.h (original)
> +++ cfe/trunk/include/clang/Lex/ModuleMap.h Tue Sep  5 14:46:22 2017
> @@ -82,22 +82,26 @@ class ModuleMap {
>
>    /// \brief The directory used for Clang-supplied, builtin include
> headers,
>    /// such as "stdint.h".
> -  const DirectoryEntry *BuiltinIncludeDir;
> +  const DirectoryEntry *BuiltinIncludeDir = nullptr;
>
>    /// \brief Language options used to parse the module map itself.
>    ///
>    /// These are always simple C language options.
>    LangOptions MMapLangOpts;
>
> -  // The module that the main source file is associated with (the module
> -  // named LangOpts::CurrentModule, if we've loaded it).
> -  Module *SourceModule;
> +  /// The module that the main source file is associated with (the module
> +  /// named LangOpts::CurrentModule, if we've loaded it).
> +  Module *SourceModule = nullptr;
> +
> +  /// The global module for the current TU, if we still own it.
> (Ownership is
> +  /// transferred if/when we create an enclosing module.
> +  std::unique_ptr<Module> PendingGlobalModule;
>
>    /// \brief The top-level modules that are known.
>    llvm::StringMap<Module *> Modules;
>
>    /// \brief The number of modules we have created in total.
> -  unsigned NumCreatedModules;
> +  unsigned NumCreatedModules = 0;
>
>  public:
>    /// \brief Flags describing the role of a module header.
>
> Modified: cfe/trunk/lib/Lex/ModuleMap.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=312580&r1=312579&r2=312580&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Lex/ModuleMap.cpp (original)
> +++ cfe/trunk/lib/Lex/ModuleMap.cpp Tue Sep  5 14:46:22 2017
> @@ -256,8 +256,7 @@ ModuleMap::ModuleMap(SourceManager &Sour
>                       const LangOptions &LangOpts, const TargetInfo
> *Target,
>                       HeaderSearch &HeaderInfo)
>      : SourceMgr(SourceMgr), Diags(Diags), LangOpts(LangOpts),
> Target(Target),
> -      HeaderInfo(HeaderInfo), BuiltinIncludeDir(nullptr),
> -      SourceModule(nullptr), NumCreatedModules(0) {
> +      HeaderInfo(HeaderInfo) {
>    MMapLangOpts.LineComment = true;
>  }
>
> @@ -747,10 +746,12 @@ std::pair<Module *, bool> ModuleMap::fin
>  }
>
>  Module *ModuleMap::createGlobalModuleForInterfaceUnit(SourceLocation Loc)
> {
> -  auto *Result = new Module("<global>", Loc, nullptr, /*IsFramework*/
> false,
> -                            /*IsExplicit*/ true, NumCreatedModules++);
> -  Result->Kind = Module::GlobalModuleFragment;
> -  return Result;
> +  assert(!PendingGlobalModule && "created multiple global modules");
> +  PendingGlobalModule.reset(
> +      new Module("<global>", Loc, nullptr, /*IsFramework*/ false,
> +                 /*IsExplicit*/ true, NumCreatedModules++));
> +  PendingGlobalModule->Kind = Module::GlobalModuleFragment;
> +  return PendingGlobalModule.get();
>  }
>
>  Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc,
> @@ -766,7 +767,10 @@ Module *ModuleMap::createModuleForInterf
>    Modules[Name] = SourceModule = Result;
>
>    // Reparent the current global module fragment as a submodule of this
> module.
> +  assert(GlobalModule == PendingGlobalModule.get() &&
> +         "unexpected global module");
>    GlobalModule->setParent(Result);
> +  PendingGlobalModule.release(); // now owned by parent
>

Would it be reasonable to reverse the 'setParent' API (into an addSubmodule
call on the parent - that takes the child/submodule by std::unique_ptr
value) so the hand-off/ownership is more clear/less error prone?


>
>    // Mark the main source file as being within the newly-created module
> so that
>    // declarations and macros are properly visibility-restricted to it.
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170911/84b3cdab/attachment.html>