[lld] [clang] [llvm] [LTO] Improve diagnostics handling when parsing module-level inline assembly (PR #75726)

2023-12-17 Thread Yuanfang Chen via cfe-commits 

https://github.com/yuanfang-chen approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/75726
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [LTO] Improve diagnostics handling when parsing module-level inline assembly (PR #75726)

2023-12-17 Thread Yuanfang Chen via cfe-commits 

yuanfang-chen wrote:

`MachO/lto-module-asm-err.ll` needs update?

https://github.com/llvm/llvm-project/pull/75726
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [LTO] Improve diagnostics handling when parsing module-level inline assembly (PR #75726)

2023-12-16 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)


Changes

Non-LTO compiles set the buffer name to ""
(`AsmPrinter::addInlineAsmDiagBuffer`) and pass diagnostics to
`ClangDiagnosticHandler` (through the `MCContext` handler in
`MachineModuleInfoWrapperPass::doInitialization`) to ensure that
the exit code is 1 in the presence of errors. In contrast, LTO compiles
spuriously succeed even if error messages are printed.

```
% cat a.c
void _start() {}
asm("unknown instruction");
% clang -c a.c
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 error generated.
% clang -c -flto a.c; echo $?  # -flto=thin is the same
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
0
```

`CollectAsmSymbols` parses inline assembly and is transitively called by
both `ModuleSummaryIndexAnalysis::run` and `WriteBitcodeToFile`, leading
to duplicate diagnostics.

This patch updates `CollectAsmSymbols` to be similar to non-LTO
compiles.
```
% clang -c -flto=thin a.c; echo $?
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 errors generated.
1
```

The `HasErrors` check does not prevent duplicate warnings but assembler
warnings are very uncommon.


---
Full diff: https://github.com/llvm/llvm-project/pull/75726.diff


4 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenAction.cpp (+2) 
- (modified) clang/test/CodeGen/invalid_global_asm.c (+5) 
- (modified) llvm/include/llvm/IR/DiagnosticHandler.h (+1) 
- (modified) llvm/lib/Object/ModuleSymbolTable.cpp (+15-1) 


``diff
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp 
b/clang/lib/CodeGen/CodeGenAction.cpp
index 753a8fd74fa696..4121a3709bc3af 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -418,6 +418,8 @@ void BackendConsumer::anchor() { }
 } // namespace clang
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
+  if (DI.getSeverity() == DS_Error)
+HasErrors = true;
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
 }
diff --git a/clang/test/CodeGen/invalid_global_asm.c 
b/clang/test/CodeGen/invalid_global_asm.c
index 5b7e8b43d752d8..d5645f7fc92bf4 100644
--- a/clang/test/CodeGen/invalid_global_asm.c
+++ b/clang/test/CodeGen/invalid_global_asm.c
@@ -1,5 +1,10 @@
 // REQUIRES: arm-registered-target
 // RUN: not %clang_cc1 -emit-obj -triple armv6-unknown-unknown -o %t %s 2>&1 | 
FileCheck %s
+// RUN: not %clang_cc1 -emit-obj -triple armv6-unknown-unknown -flto -o %t %s 
2>&1 | FileCheck %s
+
+/// Test the diagnostic behavior considering the whole system including the 
driver.
+// RUN: not %clang --target=armv6-unknown-unknown -c -flto=thin -o %t %s 2>&1 
| FileCheck %s
 #pragma clang diagnostic ignored "-Wmissing-noreturn"
 __asm__(".Lfoo: movw r2, #:lower16:.Lbar - .Lfoo");
 // CHECK: :1:8: error: instruction requires: armv6t2
+// CHECK-NOT: error:
diff --git a/llvm/include/llvm/IR/DiagnosticHandler.h 
b/llvm/include/llvm/IR/DiagnosticHandler.h
index 55e5e5975808d9..db7d7444f75f05 100644
--- a/llvm/include/llvm/IR/DiagnosticHandler.h
+++ b/llvm/include/llvm/IR/DiagnosticHandler.h
@@ -23,6 +23,7 @@ class DiagnosticInfo;
 /// which remarks are enabled.
 struct DiagnosticHandler {
   void *DiagnosticContext = nullptr;
+  bool HasErrors = false;
   DiagnosticHandler(void *DiagContext = nullptr)
   : DiagnosticContext(DiagContext) {}
   virtual ~DiagnosticHandler() = default;
diff --git a/llvm/lib/Object/ModuleSymbolTable.cpp 
b/llvm/lib/Object/ModuleSymbolTable.cpp
index ab073e18cb4668..07f76688fa43e7 100644
--- a/llvm/lib/Object/ModuleSymbolTable.cpp
+++ b/llvm/lib/Object/ModuleSymbolTable.cpp
@@ -16,6 +16,7 @@
 #include "RecordStreamer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalValue.h"
@@ -68,6 +69,11 @@ void ModuleSymbolTable::addModule(Module *M) {
 static void
 initializeRecordStreamer(const Module &M,
  function_ref Init) {
+  // This function may be called twice, once for ModuleSummaryIndexAnalysis and
+  // the other when writing the IR symbol table. If parsing inline assembly has
+  // caused errors in the first run, suppress the second run.
+  if (M.getContext().getDiagHandlerPtr()->HasErrors)
+return;
   StringRef InlineAsm = M.getModuleInlineAsm();
   if (InlineAsm.empty())
 return;
@@ -95,7 +101,8 @@ initializeRecordStreamer(const Module &M,
   if (!MCII)
 return;
 
-  std::unique_ptr Buffer(MemoryBuffer::getMemBuffer(InlineAsm));
+  std::unique_ptr Buffer(
+  MemoryBuffer::getMemBuffer(InlineAsm, ""));
   SourceMgr SrcMgr;
   SrcMgr.AddNewSourceBuffer(std::move(Buffer), SMLoc());
 
@@ -115,6 +122,13 @@ initializeRecordStreamer(const Module &M,

[clang] [llvm] [LTO] Improve diagnostics handling when parsing module-level inline assembly (PR #75726)

2023-12-16 Thread Fangrui Song via cfe-commits 

https://github.com/MaskRay created 
https://github.com/llvm/llvm-project/pull/75726

Non-LTO compiles set the buffer name to ""
(`AsmPrinter::addInlineAsmDiagBuffer`) and pass diagnostics to
`ClangDiagnosticHandler` (through the `MCContext` handler in
`MachineModuleInfoWrapperPass::doInitialization`) to ensure that
the exit code is 1 in the presence of errors. In contrast, LTO compiles
spuriously succeed even if error messages are printed.

```
% cat a.c
void _start() {}
asm("unknown instruction");
% clang -c a.c
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 error generated.
% clang -c -flto a.c; echo $?  # -flto=thin is the same
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
0
```

`CollectAsmSymbols` parses inline assembly and is transitively called by
both `ModuleSummaryIndexAnalysis::run` and `WriteBitcodeToFile`, leading
to duplicate diagnostics.

This patch updates `CollectAsmSymbols` to be similar to non-LTO
compiles.
```
% clang -c -flto=thin a.c; echo $?
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 errors generated.
1
```

The `HasErrors` check does not prevent duplicate warnings but assembler
warnings are very uncommon.


>From ddce8597697261dd48ffd81761518496d391b55e Mon Sep 17 00:00:00 2001
From: Fangrui Song 
Date: Sun, 10 Dec 2023 10:13:59 -0800
Subject: [PATCH] [LTO] Improve diagnostics handling when parsing module-level
 inline assembly

Non-LTO compiles set the buffer name to ""
(`AsmPrinter::addInlineAsmDiagBuffer`) and pass diagnostics to
`ClangDiagnosticHandler` (through the `MCContext` handler in
`MachineModuleInfoWrapperPass::doInitialization`) to ensure that
the exit code is 1 in the presence of errors. In contrast, LTO compiles
spuriously succeed even if error messages are printed.

```
% cat a.c
void _start() {}
asm("unknown instruction");
% clang -c a.c
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 error generated.
% clang -c -flto a.c; echo $?  # -flto=thin is the same
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
error: invalid instruction mnemonic 'unknown'
unknown instruction
^~~
0
```

`CollectAsmSymbols` parses inline assembly and is transitively called by
both `ModuleSummaryIndexAnalysis::run` and `WriteBitcodeToFile`, leading
to duplicate diagnostics.

This patch updates `CollectAsmSymbols` to be similar to non-LTO
compiles.
```
% clang -c -flto=thin a.c; echo $?
:1:1: error: invalid instruction mnemonic 'unknown'
1 | unknown instruction
  | ^
1 errors generated.
1
```

The `HasErrors` check does not prevent duplicate warnings but assembler
warnings are very uncommon.
---
 clang/lib/CodeGen/CodeGenAction.cpp  |  2 ++
 clang/test/CodeGen/invalid_global_asm.c  |  5 +
 llvm/include/llvm/IR/DiagnosticHandler.h |  1 +
 llvm/lib/Object/ModuleSymbolTable.cpp| 16 +++-
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp 
b/clang/lib/CodeGen/CodeGenAction.cpp
index 753a8fd74fa696..4121a3709bc3af 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -418,6 +418,8 @@ void BackendConsumer::anchor() { }
 } // namespace clang
 
 bool ClangDiagnosticHandler::handleDiagnostics(const DiagnosticInfo &DI) {
+  if (DI.getSeverity() == DS_Error)
+HasErrors = true;
   BackendCon->DiagnosticHandlerImpl(DI);
   return true;
 }
diff --git a/clang/test/CodeGen/invalid_global_asm.c 
b/clang/test/CodeGen/invalid_global_asm.c
index 5b7e8b43d752d8..d5645f7fc92bf4 100644
--- a/clang/test/CodeGen/invalid_global_asm.c
+++ b/clang/test/CodeGen/invalid_global_asm.c
@@ -1,5 +1,10 @@
 // REQUIRES: arm-registered-target
 // RUN: not %clang_cc1 -emit-obj -triple armv6-unknown-unknown -o %t %s 2>&1 | 
FileCheck %s
+// RUN: not %clang_cc1 -emit-obj -triple armv6-unknown-unknown -flto -o %t %s 
2>&1 | FileCheck %s
+
+/// Test the diagnostic behavior considering the whole system including the 
driver.
+// RUN: not %clang --target=armv6-unknown-unknown -c -flto=thin -o %t %s 2>&1 
| FileCheck %s
 #pragma clang diagnostic ignored "-Wmissing-noreturn"
 __asm__(".Lfoo: movw r2, #:lower16:.Lbar - .Lfoo");
 // CHECK: :1:8: error: instruction requires: armv6t2
+// CHECK-NOT: error:
diff --git a/llvm/include/llvm/IR/DiagnosticHandler.h 
b/llvm/include/llvm/IR/DiagnosticHandler.h
index 55e5e5975808d9..db7d7444f75f05 100644
--- a/llvm/include/llvm/IR/DiagnosticHandler.h
+++ b/llvm/include/llvm/IR/DiagnosticHandler.h
@@ -23,6 +23,7 @@ class DiagnosticInfo;
 /// which remarks are enabled.
 struct DiagnosticHandler {
   void *DiagnosticContext = nullptr;
+  bool HasErrors = false;
   DiagnosticHandler(void *DiagContext = nullptr)
   : DiagnosticContext(DiagContext) {}
   virtual ~DiagnosticHandler() = default;
diff --git a/llvm/lib/Obj