[Clang] Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled

[vivekvpandya]

Summary

Clang changes related to Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled

[vivekvpandya]

Updated the patch! I don't think now we require to save OldDiagnosticHandler and context as we have ClangDiagnosticHandler class with creation of CreateASTConsumer, but I need some review.

[vivekvpandya]

Clang-formated

[anemet]

Please clean this up as well (don't have commented-out lines) so that it's ready to go with the LLVM patch.

lib/CodeGen/CodeGenAction.cpp

326–327

Remove this.

786

Again don't overload DiagContext as such. Have a dedicated field for the BackendConsumer.

881–882

Any reason you moved where we set this up?

[vivekvpandya]

lib/CodeGen/CodeGenAction.cpp

881–882

1. At older place I was not able to define ClangDiagnosticHandler class as it will require definition of BackendComsumer and vice versa.
2. and this seems to be appropriate place to create and tie DiagnosticHandler to LLVMContext

But I am not sure about why old diagnostic handler was tied back to context that is why I wanted someone from clang to look at this.

[vivekvpandya]

Update

[anemet]

lib/CodeGen/CodeGenAction.cpp

881–882

At older place I was not able to define ClangDiagnosticHandler class as it will require definition of BackendComsumer and vice versa.

Why? That was inside BackendConsumer.

But I am not sure about why old diagnostic handler was tied back to context that is why I wanted someone from clang to look at this.

You can just save the old DiagHandler object instead.

[vivekvpandya]

Why? That was inside BackendConsumer.

I was getting incomplete type error.

You can just save the old DiagHandler object instead.
I don't think now we need to do that as per my understanding CodeGen i.e emitting LLVM IR is last phase in clang which will pass control to LLVMContext. LLVMContext will have Base class of DiagnosticHandler which will not handle diagnostic and return false so LLVMContext::diagnose() will print diagnostic with

DiagnosticPrinterRawOStream DP(errs());

errs() << getDiagnosticMessagePrefix(DI.getSeverity()) << ": ";
DI.print(DP);
errs() << "\n";

and clang's diagnostic handler also does similar thing so if we keep ClangDiagnosticHandler pointer in LLVMContext it should not break.

[anemet]

In D37196#868320, @vivekvpandya wrote:

Why? That was inside BackendConsumer.

I was getting incomplete type error.

You may have to move the class declaration of ClangDiagnosticHandler before the BackendConsumer and move the definition of ClangDiagnosticHandler::handleDiagnositic out of line since that is where you use BackendConsumer. Something like:

class BackendConsumer;

class ClangDiagnosticHandler {
public:
  ...  // as before
  bool handleDiagnostics(const DiagnosticInfo &DI) override;
  ...  
private:
  ... 
}

class BackendConsumer {
  ...
}

bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
  ...
}

You can just save the old DiagHandler object instead.

I don't think now we need to do that as per my understanding CodeGen i.e emitting LLVM IR is last phase in clang which will pass control to LLVMContext. LLVMContext will have Base class of DiagnosticHandler which will not handle diagnostic and return false so LLVMContext::diagnose() will print diagnostic with

DiagnosticPrinterRawOStream DP(errs());

errs() << getDiagnosticMessagePrefix(DI.getSeverity()) << ": ";
DI.print(DP);
errs() << "\n";

and clang's diagnostic handler also does similar thing so if we keep ClangDiagnosticHandler pointer in LLVMContext it should not break.

That would have been true even before and we still restored it, no? I guess I am not sure why we used to restore this so I would just do it regardless and not change functionality.

If you want we can separately take a look whether we need to restore these original handlers.

[vivekvpandya]

update

[anemet]

LGTM.

[shafik]

It looks like these changes are already landed so closing

lib/CodeGen/CodeGenAction.cpp

 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Transforms/IPO/Internalize.h"
 
 #include <memory>
 using namespace clang;
 using namespace llvm;
 
 namespace clang {
+  class BackendConsumer;
+  class ClangDiagnosticHandler final : public DiagnosticHandler {
+  public:
+    ClangDiagnosticHandler(const CodeGenOptions &CGOpts, BackendConsumer *BCon)
+        : CodeGenOpts(CGOpts), BackendCon(BCon) {}
+  
+    bool handleDiagnostics(const DiagnosticInfo &DI) override;
+    bool isAnalysisRemarkEnable(const std::string &PassName) {
+      return (CodeGenOpts.OptimizationRemarkAnalysisPattern &&
+              CodeGenOpts.OptimizationRemarkAnalysisPattern->match(PassName));
+    }
+    bool isMissedOptRemarkEnable(const std::string &PassName) {
+      return (CodeGenOpts.OptimizationRemarkMissedPattern &&
+              CodeGenOpts.OptimizationRemarkMissedPattern->match(PassName));
+    }
+    bool isPassedOptRemarkEnable(const std::string &PassName) {
+      return (CodeGenOpts.OptimizationRemarkPattern &&
+              CodeGenOpts.OptimizationRemarkPattern->match(PassName));
+    }
+  
+  private:
+    const CodeGenOptions &CodeGenOpts;
+    BackendConsumer *BackendCon;
+  };
+
   class BackendConsumer : public ASTConsumer {
     using LinkModule = CodeGenAction::LinkModule;
 
     virtual void anchor();
     DiagnosticsEngine &Diags;
     BackendAction Action;
     const HeaderSearchOptions &HeaderSearchOpts;
     const CodeGenOptions &CodeGenOpts;
IRGenFinished = true;
       // Install an inline asm handler so that diagnostics get printed through
       // our diagnostics hooks.
       LLVMContext &Ctx = getModule()->getContext();
       LLVMContext::InlineAsmDiagHandlerTy OldHandler =
         Ctx.getInlineAsmDiagnosticHandler();
       void *OldContext = Ctx.getInlineAsmDiagnosticContext();
       Ctx.setInlineAsmDiagnosticHandler(InlineAsmDiagHandler, this);
 
-      LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
+      std::unique_ptr<DiagnosticHandler> OldDiagnosticHandler =
           Ctx.getDiagnosticHandler();
-      void *OldDiagnosticContext = Ctx.getDiagnosticContext();
-      Ctx.setDiagnosticHandler(DiagnosticHandler, this);
+      Ctx.setDiagnosticHandler(llvm::make_unique<ClangDiagnosticHandler>(
+        CodeGenOpts, this));
       Ctx.setDiagnosticsHotnessRequested(CodeGenOpts.DiagnosticsWithHotness);
       if (CodeGenOpts.DiagnosticsHotnessThreshold != 0)
         Ctx.setDiagnosticsHotnessThreshold(
             CodeGenOpts.DiagnosticsHotnessThreshold);
 
       std::unique_ptr<llvm::tool_output_file> OptRecordFile;
       if (!CodeGenOpts.OptRecordFile.empty()) {
         std::error_code EC;
IRGenFinished = true;
       EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef());
 
       EmitBackendOutput(Diags, HeaderSearchOpts, CodeGenOpts, TargetOpts,
                         LangOpts, C.getTargetInfo().getDataLayout(),
                         getModule(), Action, std::move(AsmOutStream));
 
       Ctx.setInlineAsmDiagnosticHandler(OldHandler, OldContext);
 
-      Ctx.setDiagnosticHandler(OldDiagnosticHandler, OldDiagnosticContext);
+      Ctx.setDiagnosticHandler(std::move(OldDiagnosticHandler));
 
       if (OptRecordFile)
         OptRecordFile->keep();
     }
 
     void HandleTagDeclDefinition(TagDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                      Context->getSourceManager(),
void HandleVTable(CXXRecordDecl *RD) override {
       Gen->HandleVTable(RD);
     }
 
     static void InlineAsmDiagHandler(const llvm::SMDiagnostic &SM,void *Context,
                                      unsigned LocCookie) {
       SourceLocation Loc = SourceLocation::getFromRawEncoding(LocCookie);
       ((BackendConsumer*)Context)->InlineAsmDiagHandler2(SM, Loc);
     }
 
-    static void DiagnosticHandler(const llvm::DiagnosticInfo &DI,
-                                  void *Context) {
-      ((BackendConsumer *)Context)->DiagnosticHandlerImpl(DI);
-    }
-
     /// Get the best possible source location to represent a diagnostic that
     /// may have associated debug info.
     const FullSourceLoc
     getBestLocationFromDebugLoc(const llvm::DiagnosticInfoWithLocationBase &D,
                                 bool &BadDebugInfo, StringRef &Filename,
                                 unsigned &Line, unsigned &Column) const;
 
     void InlineAsmDiagHandler2(const llvm::SMDiagnostic &,
                                SourceLocation LocCookie);
void OptimizationRemarkHandler(
         const llvm::OptimizationRemarkAnalysisAliasing &D);
     void OptimizationFailureHandler(
         const llvm::DiagnosticInfoOptimizationFailure &D);
   };
 
   void BackendConsumer::anchor() {}
 }
 
+bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
+  BackendCon->DiagnosticHandlerImpl(DI);
+  return true;
+}
+
 /// ConvertBackendLocation - Convert a location in a temporary llvm::SourceMgr
 /// buffer to be a valid FullSourceLoc.
 static FullSourceLoc ConvertBackendLocation(const llvm::SMDiagnostic &D,
                                             SourceManager &CSM) {
   // Get both the clang and llvm source managers.  The location is relative to
   // a memory buffer that the LLVM Source Manager is handling, we need to add
   // a copy to the Clang source manager.
   const llvm::SourceMgr &LSM = *D.getSourceMgr();
void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {
   // Report the backend message using the usual diagnostic mechanism.
   FullSourceLoc Loc;
   Diags.Report(Loc, DiagID).AddString(MsgStorage);
 }
 #undef ComputeDiagID
 
 CodeGenAction::CodeGenAction(unsigned _Act, LLVMContext *_VMContext)
     : Act(_Act), VMContext(_VMContext ? _VMContext : new LLVMContext),
       OwnsVMContext(!_VMContext) {}
 
 CodeGenAction::~CodeGenAction() {
   TheModule.reset();
   if (OwnsVMContext)
     delete VMContext;
 }
 
 bool CodeGenAction::hasIRSupport() const { return true; }
CodeGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
   }
 
   std::unique_ptr<BackendConsumer> Result(new BackendConsumer(
       BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),
       CI.getPreprocessorOpts(), CI.getCodeGenOpts(), CI.getTargetOpts(),
       CI.getLangOpts(), CI.getFrontendOpts().ShowTimers, InFile,
       std::move(LinkModules), std::move(OS), *VMContext, CoverageInfo));
   BEConsumer = Result.get();
 
   // Enable generating macro debug info only when debug info is not disabled and
   // also macro debug info is enabled.
   if (CI.getCodeGenOpts().getDebugInfo() != codegenoptions::NoDebugInfo &&
       CI.getCodeGenOpts().MacroDebugInfo) {
     std::unique_ptr<PPCallbacks> Callbacks =
         llvm::make_unique<MacroPPCallbacks>(BEConsumer->getCodeGenerator(),
                                             CI.getPreprocessor());
     CI.getPreprocessor().addPPCallbacks(std::move(Callbacks));
   }