[Dwarf][Transforms] Add dwarf support when func signature changed #127855
Conversation
yonghong-song
requested review from
arsenm,
nikic,
eddyz87,
snehasish,
mtrofin,
jdoerfert,
4ast,
teresajohnson,
ayermolo and
alx32
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-binary-utilities Author: None (yonghong-song) ChangesThe goal is to add additional tag/attribute to dwarf so users will know that function signatures get changed. See [1] for motivation. Otherwise, users may assume function signature remaining the same as its source, and bpf tracing may get wrong results. With explicit tag/attribute in dwarf to indicate a func signature change, for bpf tracing, users will either go into the asm code to find the exact signature or go to find another non-signature-change function for tracing, instead of debugging the otherwise rong results. Earlier I have a pull request [2] attempts to add suffix to indicate signature change as gcc did this already. But later upstream suggested to use dwarf to encode such suffix change info ([1]). This patch introduced a new tag LLVM_func_args_changed and a new attr LLVM_func_retval_removed. In DeadArgumentElimination pass, if a function return value is removed, LLVM_func_retval_removed attr will be added to that func in the dwarf. In DeadArgumentElimination and ArgumentPromotion passes, if the function signature is changed, LLVM_func_args_changed tag is added to dwarf. Here, LLVM_func_args_changed tag is used so later on, we could add more debug info about what changes. Regarding to potential more info under LLVM_func_args_changed, we might need the following info.
[1] https://discourse.llvm.org/t/rfc-identify-func-signature-change-in-llvm-compiled-kernel-image/82609 Patch is 22.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127855.diff 10 Files Affected:
diff --git a/llvm/include/llvm/BinaryFormat/Dwarf.def b/llvm/include/llvm/BinaryFormat/Dwarf.def
index 724a14ccc7aea..de4215e088dd5 100644
--- a/llvm/include/llvm/BinaryFormat/Dwarf.def
+++ b/llvm/include/llvm/BinaryFormat/Dwarf.def
@@ -268,6 +268,7 @@ HANDLE_DW_TAG(0x5111, ALTIUM_rom, 0, ALTIUM, DW_KIND_NONE)
// LLVM
HANDLE_DW_TAG(0x6000, LLVM_annotation, 0, LLVM, DW_KIND_NONE)
+HANDLE_DW_TAG(0x6001, LLVM_func_args_changed, 0, LLVM, DW_KIND_NONE)
// Green Hills.
HANDLE_DW_TAG(0x8004, GHS_namespace, 0, GHS, DW_KIND_NONE)
@@ -624,6 +625,7 @@ HANDLE_DW_AT(0x3e09, LLVM_ptrauth_authenticates_null_values, 0, LLVM)
HANDLE_DW_AT(0x3e0a, LLVM_ptrauth_authentication_mode, 0, LLVM)
HANDLE_DW_AT(0x3e0b, LLVM_num_extra_inhabitants, 0, LLVM)
HANDLE_DW_AT(0x3e0c, LLVM_stmt_sequence, 0, LLVM)
+HANDLE_DW_AT(0x3e0d, LLVM_func_retval_removed, 0, LLVM)
// Apple extensions.
diff --git a/llvm/include/llvm/IR/DebugInfoFlags.def b/llvm/include/llvm/IR/DebugInfoFlags.def
index df375b6c68e81..e693addab7879 100644
--- a/llvm/include/llvm/IR/DebugInfoFlags.def
+++ b/llvm/include/llvm/IR/DebugInfoFlags.def
@@ -91,11 +91,13 @@ HANDLE_DISP_FLAG((1u << 8), MainSubprogram)
// for defaulted functions
HANDLE_DISP_FLAG((1u << 9), Deleted)
HANDLE_DISP_FLAG((1u << 11), ObjCDirect)
+HANDLE_DISP_FLAG((1u << 12), ArgChanged)
+HANDLE_DISP_FLAG((1u << 13), RetvalRemoved)
#ifdef DISP_FLAG_LARGEST_NEEDED
// Intended to be used with ADT/BitmaskEnum.h.
// NOTE: Always must be equal to largest flag, check this when adding new flags.
-HANDLE_DISP_FLAG((1 << 11), Largest)
+HANDLE_DISP_FLAG((1 << 13), Largest)
#undef DISP_FLAG_LARGEST_NEEDED
#endif
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 8515d8eda8568..73d568b5576b4 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -1907,6 +1907,11 @@ class DISubprogram : public DILocalScope {
DIScope *getScope() const { return cast_or_null<DIScope>(getRawScope()); }
+ void setArgChanged() { SPFlags |= SPFlagArgChanged; }
+ bool getArgChanged() const { return SPFlags & SPFlagArgChanged; }
+ void setRetvalRemoved() { SPFlags |= SPFlagRetvalRemoved; }
+ bool getRetvalRemoved() const { return SPFlags & SPFlagRetvalRemoved; }
+
StringRef getName() const { return getStringOperand(2); }
StringRef getLinkageName() const { return getStringOperand(3); }
/// Only used by clients of CloneFunction, and only right after the cloning.
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index ddf0275ddfe6a..a80d764683db7 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -1123,6 +1123,8 @@ DIE &DwarfCompileUnit::constructSubprogramScopeDIE(const DISubprogram *Sub,
MCSymbol *LineTableSym) {
DIE &ScopeDIE = updateSubprogramScopeDIE(Sub, LineTableSym);
+ DwarfUnit::addLLVMChangedArgs(ScopeDIE, Sub);
+
if (Scope) {
assert(!Scope->getInlinedAt());
assert(!Scope->isAbstractScope());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
index 5347c8a049ba6..ab97d5c41a2f3 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -654,6 +654,14 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const MDNode *TyNode) {
->createTypeDIE(Context, *ContextDIE, Ty);
}
+void DwarfUnit::addLLVMChangedArgs(DIE &ScopeDIE, const DISubprogram *SP) {
+ if (!SP->getArgChanged())
+ return;
+
+ auto *LocalDie = DIE::get(DIEValueAllocator, dwarf::DW_TAG_LLVM_func_args_changed);
+ ScopeDIE.addChild(LocalDie);
+}
+
void DwarfUnit::updateAcceleratorTables(const DIScope *Context,
const DIType *Ty, const DIE &TyDIE) {
if (Ty->getName().empty())
@@ -1328,6 +1336,9 @@ void DwarfUnit::applySubprogramAttributes(const DISubprogram *SP, DIE &SPDie,
if (!SkipSPSourceLocation)
addSourceLine(SPDie, SP);
+ if (SP->getRetvalRemoved())
+ addFlag(SPDie, dwarf::DW_AT_LLVM_func_retval_removed);
+
// Skip the rest of the attributes under -gmlt to save space.
if (SkipSPAttributes)
return;
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
index 9ddd6f8c14175..49724bcfc87ff 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
@@ -324,6 +324,9 @@ class DwarfUnit : public DIEUnit {
void updateAcceleratorTables(const DIScope *Context, const DIType *Ty,
const DIE &TyDIE);
+ /// Add DW_TAG_LLVM_func_args_changed.
+ void addLLVMChangedArgs(DIE &ScopeDIE, const DISubprogram *SP);
+
protected:
~DwarfUnit();
diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
index c440638884322..e81adb830a86b 100644
--- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -51,6 +51,7 @@
#include "llvm/IR/CFG.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Function.h"
@@ -132,6 +133,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,
// First, determine the new argument list
unsigned ArgNo = 0, NewArgNo = 0;
+ bool CurrFuncArgChanged = false;
for (Function::arg_iterator I = F->arg_begin(), E = F->arg_end(); I != E;
++I, ++ArgNo) {
if (!ArgsToPromote.count(&*I)) {
@@ -142,6 +144,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,
} else if (I->use_empty()) {
// Dead argument (which are always marked as promotable)
++NumArgumentsDead;
+ CurrFuncArgChanged = true;
ORE.emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "ArgumentRemoved", F)
<< "eliminating argument " << ore::NV("ArgName", I->getName())
@@ -156,6 +159,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,
ArgAttrVec.push_back(AttributeSet());
}
++NumArgumentsPromoted;
+ CurrFuncArgChanged = true;
ORE.emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "ArgumentPromoted", F)
<< "promoting argument " << ore::NV("ArgName", I->getName())
@@ -433,6 +437,10 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,
PromoteMemToReg(Allocas, DT, &AC);
}
+ DISubprogram *SP = NF->getSubprogram();
+ if (SP && CurrFuncArgChanged)
+ SP->setArgChanged();
+
return NF;
}
diff --git a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
index ed93b4491c50e..5df3d47daa8dd 100644
--- a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -757,6 +757,8 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
// a new set of parameter attributes to correspond. Skip the first parameter
// attribute, since that belongs to the return value.
unsigned ArgI = 0;
+ bool CurrFuncArgEliminated = false;
+ bool CurrFuncRetEliminated = false;
for (Function::arg_iterator I = F->arg_begin(), E = F->arg_end(); I != E;
++I, ++ArgI) {
RetOrArg Arg = createArg(F, ArgI);
@@ -767,6 +769,7 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
HasLiveReturnedArg |= PAL.hasParamAttr(ArgI, Attribute::Returned);
} else {
++NumArgumentsEliminated;
+ CurrFuncArgEliminated = true;
ORE.emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "ArgumentRemoved", F)
@@ -818,6 +821,7 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
NewRetIdxs[Ri] = RetTypes.size() - 1;
} else {
++NumRetValsEliminated;
+ CurrFuncRetEliminated = true;
ORE.emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "ReturnValueRemoved", F)
@@ -1099,6 +1103,12 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
// to call this function or try to interpret the return value.
if (NFTy != FTy && NF->getSubprogram()) {
DISubprogram *SP = NF->getSubprogram();
+
+ if (CurrFuncArgEliminated)
+ SP->setArgChanged();
+ if (CurrFuncRetEliminated)
+ SP->setRetvalRemoved();
+
auto Temp = SP->getType()->cloneWithCC(llvm::dwarf::DW_CC_nocall);
SP->replaceType(MDNode::replaceWithPermanent(std::move(Temp)));
}
diff --git a/llvm/test/DebugInfo/arg-prom.ll b/llvm/test/DebugInfo/arg-prom.ll
new file mode 100644
index 0000000000000..dd8c91ec2438e
--- /dev/null
+++ b/llvm/test/DebugInfo/arg-prom.ll
@@ -0,0 +1,157 @@
+; REQUIRES: x86-registered-target
+; RUN: opt -O3 -S < %s | FileCheck %s
+;
+; Source code:
+; __attribute__((noinline)) static int is_absolute_path(const char *path)
+; {
+; return path[0] == '/';
+; }
+;
+; int scpy(char *, const char *, int);
+; int quit(void);
+; const char *make_nonrelative_path(char *buf, int sz, const char *path)
+; {
+; if (is_absolute_path(path)) {
+; if (scpy(buf, path, sz) >= sz)
+; quit();
+; }
+; return buf;
+; }
+; Compilation flag:
+; clang -O2 -g -S -emit-llvm -Xclang -disable-llvm-passes test.c
+
+; ModuleID = 'test.c'
+source_filename = "test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define dso_local ptr @make_nonrelative_path(ptr noundef %0, i32 noundef %1, ptr noundef %2) #0 !dbg !9 {
+ %4 = alloca ptr, align 8
+ %5 = alloca i32, align 4
+ %6 = alloca ptr, align 8
+ store ptr %0, ptr %4, align 8, !tbaa !21
+ #dbg_declare(ptr %4, !18, !DIExpression(), !26)
+ store i32 %1, ptr %5, align 4, !tbaa !27
+ #dbg_declare(ptr %5, !19, !DIExpression(), !29)
+ store ptr %2, ptr %6, align 8, !tbaa !21
+ #dbg_declare(ptr %6, !20, !DIExpression(), !30)
+ %7 = load ptr, ptr %6, align 8, !dbg !31, !tbaa !21
+ %8 = call i32 @is_absolute_path(ptr noundef %7), !dbg !33
+ %9 = icmp ne i32 %8, 0, !dbg !33
+ br i1 %9, label %10, label %20, !dbg !33
+
+10: ; preds = %3
+ %11 = load ptr, ptr %4, align 8, !dbg !34, !tbaa !21
+ %12 = load ptr, ptr %6, align 8, !dbg !37, !tbaa !21
+ %13 = load i32, ptr %5, align 4, !dbg !38, !tbaa !27
+ %14 = call i32 @scpy(ptr noundef %11, ptr noundef %12, i32 noundef %13), !dbg !39
+ %15 = load i32, ptr %5, align 4, !dbg !40, !tbaa !27
+ %16 = icmp sge i32 %14, %15, !dbg !41
+ br i1 %16, label %17, label %19, !dbg !41
+
+17: ; preds = %10
+ %18 = call i32 @quit(), !dbg !42
+ br label %19, !dbg !42
+
+19: ; preds = %17, %10
+ br label %20, !dbg !43
+
+20: ; preds = %19, %3
+ %21 = load ptr, ptr %4, align 8, !dbg !44, !tbaa !21
+ ret ptr %21, !dbg !45
+}
+
+; Function Attrs: noinline nounwind uwtable
+define internal i32 @is_absolute_path(ptr noundef %0) #1 !dbg !46 {
+ %2 = alloca ptr, align 8
+ store ptr %0, ptr %2, align 8, !tbaa !21
+ #dbg_declare(ptr %2, !50, !DIExpression(), !51)
+ %3 = load ptr, ptr %2, align 8, !dbg !52, !tbaa !21
+ %4 = getelementptr inbounds i8, ptr %3, i64 0, !dbg !52
+ %5 = load i8, ptr %4, align 1, !dbg !52, !tbaa !53
+ %6 = sext i8 %5 to i32, !dbg !52
+ %7 = icmp eq i32 %6, 47, !dbg !54
+ %8 = zext i1 %7 to i32, !dbg !54
+ ret i32 %8, !dbg !55
+}
+
+; CHECK: define internal fastcc range(i32 0, 2) i32 @is_absolute_path(i8 {{.*}})
+
+declare !dbg !56 i32 @scpy(ptr noundef, ptr noundef, i32 noundef) #2
+
+declare !dbg !59 i32 @quit() #2
+
+attributes #0 = { nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { noinline nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 21.0.0git ([email protected]:yonghong-song/llvm-project.git bbfd0a15ade80596f6d6dde8add7d50f4875dde1)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/tmp/tests/sig-change/prom", checksumkind: CSK_MD5, checksum: "1befb35eb4507489630adb56cb20fe09")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{!"clang version 21.0.0git ([email protected]:yonghong-song/llvm-project.git bbfd0a15ade80596f6d6dde8add7d50f4875dde1)"}
+!9 = distinct !DISubprogram(name: "make_nonrelative_path", scope: !1, file: !1, line: 8, type: !10, scopeLine: 9, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !17)
+!10 = !DISubroutineType(types: !11)
+!11 = !{!12, !15, !16, !12}
+!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
+!13 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !14)
+!14 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!15 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !14, size: 64)
+!16 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!17 = !{!18, !19, !20}
+!18 = !DILocalVariable(name: "buf", arg: 1, scope: !9, file: !1, line: 8, type: !15)
+!19 = !DILocalVariable(name: "sz", arg: 2, scope: !9, file: !1, line: 8, type: !16)
+!20 = !DILocalVariable(name: "path", arg: 3, scope: !9, file: !1, line: 8, type: !12)
+!21 = !{!22, !22, i64 0}
+!22 = !{!"p1 omnipotent char", !23, i64 0}
+!23 = !{!"any pointer", !24, i64 0}
+!24 = !{!"omnipotent char", !25, i64 0}
+!25 = !{!"Simple C/C++ TBAA"}
+!26 = !DILocation(line: 8, column: 41, scope: !9)
+!27 = !{!28, !28, i64 0}
+!28 = !{!"int", !24, i64 0}
+!29 = !DILocation(line: 8, column: 50, scope: !9)
+!30 = !DILocation(line: 8, column: 66, scope: !9)
+!31 = !DILocation(line: 10, column: 30, scope: !32)
+!32 = distinct !DILexicalBlock(scope: !9, file: !1, line: 10, column: 13)
+!33 = !DILocation(line: 10, column: 13, scope: !32)
+!34 = !DILocation(line: 11, column: 26, scope: !35)
+!35 = distinct !DILexicalBlock(scope: !36, file: !1, line: 11, column: 21)
+!36 = distinct !DILexicalBlock(scope: !32, file: !1, line: 10, column: 37)
+!37 = !DILocation(line: 11, column: 31, scope: !35)
+!38 = !DILocation(line: 11, column: 37, scope: !35)
+!39 = !DILocation(line: 11, column: 21, scope: !35)
+!40 = !DILocation(line: 11, column: 44, scope: !35)
+!41 = !DILocation(line: 11, column: 41, scope: !35)
+!42 = !DILocation(line: 12, column: 4, scope: !35)
+!43 = !DILocation(line: 13, column: 9, scope: !36)
+!44 = !DILocation(line: 14, column: 16, scope: !9)
+!45 = !DILocation(line: 14, column: 9, scope: !9)
+!46 = distinct !DISubprogram(name: "is_absolute_path", scope: !1, file: !1, line: 1, type: !47, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !49)
+
+; CHECK: distinct !DISubprogram(name: "is_absolute_path", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[#]], scopeLine: [[#]], flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized | DISPFlagArgChanged, unit: ![[#]], retainedNodes: ![[#]])
+
+!47 = !DISubroutineType(types: !48)
+!48 = !{!16, !12}
+!49 = !{!50}
+!50 = !DILocalVariable(name: "path", arg: 1, scope: !46, file: !1, line: 1, type: !12)
+!51 = !DILocation(line: 1, column: 67, scope: !46)
+!52 = !DILocation(line: 3, column: 16, scope: !46)
+!53 = !{!24, !24, i64 0}
+!54 = !DILocation(line: 3, column: 24, scope: !46)
+!55 = !DILocation(line: 3, column: 9, scope: !46)
+!56 = !DISubprogram(name: "scpy", scope: !1, file: !1, line: 6, type: !57, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!57 = !DISubroutineType(types: !58)
+!58 = !{!16, !15, !12, !16}
+!59 = !DISubprogram(name: "quit", scope: !1, file: !1, line: 7, type: !60, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!60 = !DISubroutineType(types: !61)
+!61 = !{!16}
diff --git a/llvm/test/DebugInfo/arg-retval-elim.ll b/llvm/test/DebugInfo/arg-retval-elim.ll
new file mode 100644
index 0000000000000..abb1ea5e06563
--- /dev/null
+++ b/llvm/test/DebugInfo/arg-retval-elim.ll
@@ -0,0 +1,103 @@
+; REQUIRES: x86-registered-target
+; RUN: opt -O2 -S < %s | FileCheck %s
+;
+; Source code:
+; int tar(int a);
+; __attribute__((noinline)) static int foo(int a, int b)
+; {
+; return tar(a) + tar(a + 1);
+; }
+; int bar(int a)
+; {
+; foo(a, 1);
+; return 0;
+; }
+; Compilation flag:
+; clang -O2 -g -S -emit-llvm -Xclang -disable-llvm-passes test.c
+
+; ModuleID = 'test.c'
+source_filename = "test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define dso_local i32 @bar(i32 noundef %0) #0 !dbg !9 {
+ %2 = alloca i32, align 4
+ store i32 %0, ptr %2, align 4, !tbaa !15
+ #dbg_declare(ptr %2, !14, !DIExpression(), !19)
+ %3 = load i32, ptr %2, align 4, !dbg !20, !tbaa !15
+ %4 = call i32 @foo(i32 noundef %3, i32 noundef 1), !dbg !21
+ ret i32 0, !dbg !22
+}
+
+; Function Attrs: noinline nounwind uwtable
+define internal i32 @foo(i32 noundef %0, i32 noundef %1) #1 !dbg !23 {
+ %3 = alloca i32, align 4
+ %4 = alloca i32, align 4
+ store i32 %0, ptr %3, align 4, !tbaa !15
+ #dbg_declare(ptr %3, !27, !DIExpression(), !29)
+ store i32 %1, ptr %4, align 4, !tbaa !15
+ #dbg_declare(ptr %4, !28, !DIExpression(), !30)
+ %5 = load i32, ptr %3, align 4, !dbg !31, !tbaa !15
+ %6 = call i32 @tar(i32 noundef %5), !dbg !32
+ %7 = load i32, ptr %3, align 4, !dbg !33, !tbaa !15
+ %8 = add nsw i32 %7, 1, !dbg !34
+ %9 = call i32 @tar(i32 noundef %8), !dbg !35
+ %10 = add nsw i32 %6, %9, !dbg !36
+ ret i32 %10, !dbg !37
+}
+
+; CHECK: define internal fastcc void @foo(i32 noundef %0)
+
+declare !dbg !38 i32 @tar(i32 noundef) #2
+
+attributes #0 = { nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { noinline nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 21.0.0git ([email protected]:yonghong-song/llvm-project.git 95c942ca729f6f240c408ceeb39d2d5f10a3b0d5)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/tmp/tests/sig-change/deadret", checksumkind: CSK_MD5, checksum: "728d225e6425c104712ae21cee1db99b")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{!"clang version 21.0.0git ([email protected]:yonghong-song/llvm-p...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
yonghong-song
force-pushed
the
sig-change-stage-1
branch
from
efe2f88 to
9070f6f
Compare
llvm/test/DebugInfo/arg-prom.ll
Outdated
| store ptr %2, ptr %6, align 8, !tbaa !21 | ||
| #dbg_declare(ptr %6, !20, !DIExpression(), !30) | ||
| %7 = load ptr, ptr %6, align 8, !dbg !31, !tbaa !21 | ||
| %8 = call i32 @is_absolute_path(ptr noundef %7), !dbg !33 |
llvm/test/DebugInfo/arg-prom.ll
Outdated
| @@ -0,0 +1,157 @@ | |||
| ; REQUIRES: x86-registered-target | |||
| ; RUN: opt -O3 -S < %s | FileCheck %s | |||
There was a problem hiding this comment.
Tests should be minimal and run the one pass
llvm/test/DebugInfo/arg-prom.ll
Outdated
|
|
||
| ; Function Attrs: nounwind uwtable | ||
| define dso_local ptr @make_nonrelative_path(ptr noundef %0, i32 noundef %1, ptr noundef %2) #0 !dbg !9 { | ||
| %4 = alloca ptr, align 8 |
There was a problem hiding this comment.
This function should be simplified to show a minimal example
llvm/test/DebugInfo/arg-prom.ll
Outdated
| ; ModuleID = 'test.c' | ||
| source_filename = "test.c" | ||
| target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128" |
| @@ -0,0 +1,103 @@ | |||
| ; REQUIRES: x86-registered-target | |||
| ; RUN: opt -O2 -S < %s | FileCheck %s | |||
There was a problem hiding this comment.
Don't run the full pipeline
| @@ -0,0 +1,103 @@ | |||
| ; REQUIRES: x86-registered-target | |||
There was a problem hiding this comment.
Move to the x86 test subdirectory instead
| attributes #0 = { nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" } | ||
| attributes #1 = { noinline nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" } | ||
| attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" } |
There was a problem hiding this comment.
Remove unnecessary attributes
|
Thanks @arsenm I will try to address your all above comments in the next revision. Thanks for reviewing! |
yonghong-song
force-pushed
the
sig-change-stage-1
branch
from
9070f6f to
6924697
Compare
| define internal fastcc i32 @foo(i32 noundef %a, i32 noundef %b) unnamed_addr #1 !dbg !19 { | ||
| #dbg_value(i32 %a, !23, !DIExpression(), !25) | ||
| #dbg_value(i32 %b, !24, !DIExpression(), !25) | ||
| %3 = tail call i32 @tar(i32 noundef %a), !dbg !26 |
| @@ -0,0 +1,76 @@ | |||
| ; RUN: opt -S -passes='default<O2>,deadargelim' < %s | FileCheck %s | |||
There was a problem hiding this comment.
Should not run full -O2 pipeline
llvm/test/DebugInfo/X86/arg-prom.ll
Outdated
| @@ -0,0 +1,95 @@ | |||
| ; RUN: opt -S -passes='default<O3>,argpromotion' < %s | FileCheck %s | |||
There was a problem hiding this comment.
Should not run full pass pipeline
llvm/test/DebugInfo/X86/arg-prom.ll
Outdated
| define dso_local ptr @make_nonrelative_path(ptr noundef %buf, ptr noundef %path) local_unnamed_addr #0 !dbg !10 { | ||
| #dbg_value(ptr %buf, !18, !DIExpression(), !20) | ||
| #dbg_value(ptr %path, !19, !DIExpression(), !20) | ||
| %3 = call fastcc i32 @is_absolute_path(ptr noundef %path), !dbg !21 |
There was a problem hiding this comment.
Do we want to do this unconditionally? If BPF is the only identified consumer, maybe not. But the size cost is low, so maybe not worth the trouble to make it conditioned on tuning or some other flag.
@dwblaikie @adrian-prantl
| auto *LocalDie = | ||
| DIE::get(DIEValueAllocator, dwarf::DW_TAG_LLVM_func_args_changed); | ||
| ScopeDIE.addChild(LocalDie); | ||
| } |
There was a problem hiding this comment.
// TODO: Add specifics about what changed.
This will clarify that you expect to add more info later, and that's why it's a tag not an attribute. (Which was my first question when reading the code. It took a more careful reading of the commit message to understand the longer-term goal.)
There was a problem hiding this comment.
You are correct. The tag intends for future extensibility.
The goal is to add additional tag/attribute to dwarf so users will know
that function signatures get changed. See [1] for motivation. Otherwise,
users may assume function signature remaining the same as its source,
and bpf tracing may get wrong results. With explicit tag/attribute in
dwarf to indicate a func signature change, for bpf tracing, users will
either go into the asm code to find the exact signature or go to find
another non-signature-change function for tracing, instead of debugging
the otherwise rong results.
Earlier I have a pull request [2] attempts to add suffix to indicate
signature change as gcc did this already. But later upstream suggested
to use dwarf to encode such suffix change info ([1]).
This patch introduced a new tag LLVM_func_args_changed and a new
attr LLVM_func_retval_removed. In DeadArgumentElimination pass, if
a function return value is removed, LLVM_func_retval_removed attr will
be added to that func in the dwarf. In DeadArgumentElimination and
ArgumentPromotion passes, if the function signature is changed,
LLVM_func_args_changed tag is added to dwarf. Here, LLVM_func_args_changed
tag is used so later on, we could add more debug info about what changes.
Regarding to potential more info under LLVM_func_args_changed, we might need
the following info.
1. Trying to have a new set of formal argument types. The existing types
should be available in related Transforms passes, but will need DIBuilder
to build DIType's and looks like there is not easy DIBuilder API to do this.
2. Trying to relate old func signature (from source) to new func signature.
For example, original arg index 2 becomes new arg index 1, etc.
More complexity will come from argument promotion and struct arguments
where struct argument has size greater than an arch register size.
[1] https://discourse.llvm.org/t/rfc-identify-func-signature-change-in-llvm-compiled-kernel-image/82609
[2] llvm#109899
yonghong-song
force-pushed
the
sig-change-stage-1
branch
from
6924697 to
8c8a204
Compare
Actually no. The intended consumer is x86, aarch64, etc. bpf is able to trace functions in those architectures to gather information inside the kernel. The signature change info (in dwarf) will help bpf users to correctly write their progs in order to trace those functions. Ideally, bpf could like accurate function prototypes after transformation for those x86 etc. architectures. But the first step would to recognize that there is a signature change so users can either avoid such functions or try to dig into the asm codes by themselves to find true signature. |
I think what @pogo59 meant (please correct me if I am wrong) are consumers like lldb, gdb, BOLT, various other utilities that process dwarf information. If this TAG becomes "standard" for optimized builds, then I believe all those DWARF consumers will need to be aware of a new TAG. |
|
@ayermolo is correct about what I meant. It's an interesting case, and other consumers may well want to know about it. Exploring this with a vendor-specific implementation is the correct way forward. If it turns out to be genuinely useful, it would be worth proposing as a new feature in DWARF v6. Right now, though, my question is basically, how much size increase do we see in the DWARF with this new feature? If it's very small (maybe the optimization does not fire very often), then there's not much harm in emitting the info all the time, as the patch currently implements it. If there's a noticeable increase, though, that's a concern for some vendors and we might want to limit the situations where the info is emitted. |
|
Could we use only the |
Hi, @dwblaikie We really want to have separate attributes for argument change and return value change. For bpf tracing, tracing arguments and tracing return values are two different bpf programs and it is totally possible users only want to trace arguments, or only want to trace return values. If we do not differentiate arguments/return_values, and a user has a bpf prog to trace arguments only, the user still needs to check asm codes to find out whether arguments changed or not. This will hurt productivity. |
|
How about an extension to the standard attribute? It's a pity the DW_CC_* doesn't have a user defined extension space, but could get a bit weird and use some high-ish value to avoid colliding with likely standard features added to the enum. Perhaps you could file a DWARF issue to discuss the long-term/standardized solution to this? |
|
Wouldn't it be possible to set the existing DW_AT_artificial attribute instead of introducing vendor-specific extensions? |
not sure if that'd connote either of the properties being expressed here (uncallable, and un-printable-returns) - for instance in C++, implicit ctors, assignment operators, and dtors are artificial but callable/printable returns based on ABI, so we couldn't have debuggers assuming artificial functions can't be called/have their return values printed. |
The following are some statistics when building a linux kernel with and without this patch: For debug_info section, without this patch the size is 0xb61ff8d and with this patch the size is 0xb6207e5. So the increase is 0x858 which is tiny. |
I could do this. But one thing I want to emphasize is for tag LLVM_func_args_changed. Currently we have not figured out what sub-tags or attributes for LLVM_func_args_changed. It won't be easy to express changed arguments, relation to original arguments, etc. But using a tag allows future extension. |
.debug_abbrev also increases, by 0x1288, for a total increase of 0x1ae0. Still tiny. So, not a problem to emit this info as currently implemented. |
I am not an expert in DW_AT_artificial. In dwarf doc, I see and flag Does this enable coding can cover all the below case:
|
It might help with the review to have some idea of how that information would be conveyed. It sounds like DeadArgumentElimination simply drops an unused argument (in effect, removes it from the argument list), so that could be described with a flag on the original DW_TAG_formal_parameter (DW_AT_omitted_in_practice, or something), which avoids the need for a DW_TAG_func_args_changed. I don't know what ArgumentPromotion does so I can't suggest how to describe what it does. If it changes the argument type, we could have a DW_AT_type_in_practice that points to the type that is actually used. If we have an idea what the final DWARF would look like (acknowledging that the DWARF committee might make different decisions), that would help figure out whether DW_TAG_LLVM_func_args_changed is actually needed. |
|
What's going on here is really kind of the opposite of what DW_AT_artificial is used for. Artificial is something introduced by the compiler that isn't in the source. Here we have something removed (or modified). |
|
How would a user currently find the And the return type gets changed to If we have the two complete definitions, it would allow BPF trace to do all needed work without any flags as it could just compare the two types and see what changed, like that the function return type has changed and there is no longer an "a" argument. |
The idea, which we were briefly discussing today in the #gcc IRC channel, would be to have the signature change of some given function represented in DWARF using a DW_TAG_artificial "container" with an inlined subroutine reflecting the original signature. Something like: The signature change would be described in the formal parameters of the DW_TAG_inlined_subroutine. |
I'd suggest we should avoid trying to solve for the specific optimizations LLVM does today - and solve for the general case of "this doesn't match the source/ABI anymore" to start with, then if folks want to claw back some cases (make some things callable that aren't due to ABI changes) we can add those in later. But trying to solve for specific cases will still leave the general problem unsolved. FWIW, ArgumentPromotion does things like - change an
The DWARF reflects the source, not the optimizations. The DW_AT_locations of the parameters might reflect the new locations of the parameters (or no location if they've been optimized away) - but those locations don't say anything about the calling ABI, just where to find the variables after the prologue and before the epilogue.
Seems like it'd be a substantial diversion from existing practice - now the primary subprogram wouldn't reflect the source anymore & debuggers unaware of the extra attribute/content would render something confusing to the user (describing the function to the user in a way that doesn't match the source) - overload resolution and other features in the debugger might behave in unexpected ways. I also don't really want LLVM to be dealing with synthesizing new types, handling what it means, at a type level, to perform certain optimizations.
It's an interesting idea... I don't totally object to it. Handles the case of "you can't call this" (because it's inlined) and I suppose with the WIP DWARFv6 feature you could even describe the return value even if it's ABI-optimized away (while still being present/computable within Not sure if it helps the original author distinguish the "arguments changed" v "return type changed" & seems harder to work with - consumer has to go looking through more places, compare things, etc, compared to being told directly about the properties. I'm still inclined to an extension of DW_AT_calling_convention myself... |
Yeah this definitely would require the consumer to do some analysis, looking at the "mapping" implied by the location lists of the parameters of the inlined call. But it has the advantage of generality and at least the amount and complexity of analysis to perform (or the mere possibility of performing the analysis) would entirely depend on each consumer's requirements and each particular optimized function. (For the record, it was Richard Biener who suggested exploring the possibiilty of using DW_TAG_artificial like this, so all merit in this proposal is is his not mine.) |
|
For
For linux kernel, pahole will do analysis in dwarf to decide whether signature changed (including ret value and/or func arguments) and may encode such informaiton in BTF. So bpf tracing users won't need to look at dwarf for this. |
@jemarch Could you share a detailed dwarf info about how signature change is encoded? |
|
Can I suggest taking this (back) to either the LLVM Discourse or, better perhaps, the dwarf mailing list? |
I will try to synthesize one showing the typical constant propagation and scalar replacement of aggregates, which are the IPA optimizations that may result in changing the signature of the function. |
|
cc @WenleiHe |
@jemarch Any update on this? @clayborg in the above has a similar idea and it would be good to see what is your proposal? Thanks. |
|
I have discussed with @jemarch about one possible dwarf format which does not need new attribute/tag and yet can represent modified func signatures. And DW_AT_artificial suggests the new signature is generated by compiler optimizations and it is generated only when func signature gets changed. In this case, we need to generate dwarf for changed func signature after middle end optimization and before SelectionDAG. I see middle-end does have simple dwarf builder but I didn't find complex builder to reconstruct dwarf types based on IR types. Any suggestion on how to generate dwarf types based on IR types in middle-end? |
|
DW_AT_artificial means "this entity is not represented explicitly in the source" which is kind of the opposite of what is actually going on--it's the inlined version that's artificial. Now, I could see flagging the DW_TAG_inlined_subroutine with DW_AT_artificial, and using the normal DW_AT_specification to point back to the DW_TAG_subprogram, but still providing the parameter details on the inlined_subroutine. Maybe this was already described above, and there's a reason not to do it that way, sorry I didn't go back and reread the entire thread. |
Thanks @pogo59, you suggest having the following? For your below worry Currently, for clang, we only care the signature change but func name If func name changed, we would like just create a dwarf entry with that changed signature since changed func name will be available in elf file symtol table and we want to keep name consistent between dwarf and symbol table. What do you think my above proposal? cc @jemarch |
|
@yonghong-song yes that is what I suggested. As @dwblaikie said a while back, we should try to solve the general problem of modified ABI but the function name is the same. We should not be limited to describing what LLVM happens to do today. |
@pogo59 Good point. WDYT? |
|
Yes that lets us describe the multiple variations. One point my suggestions does not address: which functions should be callable from the debugger, and which not callable? If the original function |
@pogo59 For my above example, I assume two functions with DW_TAG_inlined_subroutine should be callable for debugger? |
The goal is to add additional tag/attribute to dwarf so users will know that function signatures get changed. See [1] for motivation. Otherwise, users may assume function signature remaining the same as its source, and bpf tracing may get wrong results. With explicit tag/attribute in dwarf to indicate a func signature change, for bpf tracing, users will either go into the asm code to find the exact signature or go to find another non-signature-change function for tracing, instead of debugging the otherwise rong results.
Note that this patch is not for BPF backend. Rather it intends to be for x86, arm64, etc since bpf tracing target is a non-bpf arch like x86/arm64/....
Earlier I have a pull request [2] attempts to add suffix to indicate signature change as gcc did this already. But later upstream suggested to use dwarf to encode such suffix change info ([1]).
This patch introduced a new tag LLVM_func_args_changed and a new attr LLVM_func_retval_removed. In DeadArgumentElimination pass, if a function return value is removed, LLVM_func_retval_removed attr will be added to that func in the dwarf. In DeadArgumentElimination and ArgumentPromotion passes, if the function signature is changed, LLVM_func_args_changed tag is added to dwarf. Here, LLVM_func_args_changed tag is used so later on, we could add more debug info about what changes.
Regarding to potential more info under LLVM_func_args_changed, we might need the following info.
[1] https://discourse.llvm.org/t/rfc-identify-func-signature-change-in-llvm-compiled-kernel-image/82609
[2] #109899