[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

In D126781#3554845 <https://reviews.llvm.org/D126781#3554845>, @rjmccall wrote:

> Okay, I understand.  So, first off, I wouldn't really call that a "weak" symbol rather than, say, a lazily-emitted symbol; "weak" already has plenty of different senses, and we should try to avoid coining more.  Also, your patch description makes it sound like there's a general bug you're fixing rather than specifically an issue with re-using a single `CodeGenerator` to generate a succession of modules; please remember that people reading your commit messages don't necessarily know you or contextualize your patches by knowing that you work on Cling.

Yes, you are right. We will update the patch title and summary.

> On a more technical note, it's now clear that the main thrust of your patch is the state persistence in `StartModule`.  Your patch is effectively adding a feature where `StartModule` can be invoked multiple times (assuming it's been appropriately finalized from earlier invocations).

Yes!

> I think that's fine, although I imagine it will need a lot of further changes to allow linkage between these modules.

Currently, we do that outside of CodeGen in a separate module pass which looks like this:

  
  bool runOnGlobal(GlobalValue& GV) {
    if (GV.isDeclaration())
      return false; // no change.
  
    // GV is a definition.
  
    // It doesn't make sense to keep unnamed constants, we wouldn't know how
    // to reference them anyway.
    if (!GV.hasName())
      return false;
  
    if (GV.getName().startswith(".str"))
      return false;
  
    llvm::GlobalValue::LinkageTypes LT = GV.getLinkage();
    if (!GV.isDiscardableIfUnused(LT))
      return false;
  
    if (LT == llvm::GlobalValue::InternalLinkage
        || LT == llvm::GlobalValue::PrivateLinkage) {
      // We want to keep this GlobalValue around, but have to tell the JIT
      // linker that it should not error on duplicate symbols.
      // FIXME: Ideally the frontend would never emit duplicate symbols and
      // we could just use the old version of saying:
      // GV.setLinkage(llvm::GlobalValue::ExternalLinkage);
      GV.setLinkage(llvm::GlobalValue::WeakAnyLinkage);
      return true; // a change!
    }
    return false;
  }

The new JIT infrastructure does not ignore duplicate symbols anymore which makes it more challenging to adjust linkage. Ideally, we should CodeGen know what's needed to make code in two different llvm::Modules work... I do not have a feeling if that requires a lot of changes or even redesign. I think this might become clearer when we start upstreaming the code "undo" patches where CodeGen's state should be reverted to a previous partial translation unit. @junaire, maybe it is a good time to make your patch in that area public and add the current reviewers.

> That doesn't really explain why you needed to add a new field to the CodeGenModule, though.

Good question. The original idea of `EmittedDeferredDecls` (patch was developed in 2017) was to track deferred decls that were emitted in a previous llvm::Module. We checked and it is not needed to fix the current test case. We also are checking on the bigger infrastructure if we can drop this field.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126781/new/

https://reviews.llvm.org/D126781