[RemoveDIs] Add flag to preserve the debug info format of input IR #87379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch adds a new flag: --preserve-input-debuginfo-format This flag instructs the tool to not convert the debug info format (intrinsics/records), but to instead determine the format of the input IR and override the other format-determining flags so that we process and output the file in the same format that we received it in. This flag is turned off by llvm-link, llvm-lto, and llvm-lto2, and should be turned off by any other tool that expects to parse multiple IR files. The motivation for this flag is to allow us to use debug records by default in all LLVM tools except for those that have an interest in not converting the debug info format - verify-uselistorder and llvm-reduce, and any downstream tools that seek to test or mutate IR as-is, without applying extraneous modifications to the input. The implementation of this flag may not be to everyone's liking - setting it to "true" directly modifies the value of the flags: --experimental-debuginfo-iterators --write-experimental-debuginfo --write-experimental-debuginfo-iterators-to-bitcode The reason for this approach is that when we pass this new flag, we're intending to override any effect that those flags might actually have; this could be done by adding a check at every site where those flags are used, but I'm planning to rewrite or remove those flags soon, so this method avoids updating the same sets of code twice without a functional change; if modifying the flag values is a problem though, then this can be rewritten.
SLTozer
requested review from
adrian-prantl,
OCHyams,
dwblaikie and
CarlosAlbertoEnciso
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Stephen Tozer (SLTozer) ChangesThis patch adds a new flag: This flag instructs the tool to not convert the debug info format (intrinsics/records) of input IR, but to instead determine the format of the input IR and overwrite the other format-determining flags so that we process and output the file in the same format that we received it in. This flag is turned off by llvm-link, llvm-lto, and llvm-lto2, and should be turned off by any other tool that expects to parse multiple IR modules and have their debug info formats match. The motivation for this flag is to allow tools to not convert the debug info format - verify-uselistorder and llvm-reduce, and any downstream tools that seek to test or mutate IR as-is, without applying extraneous modifications to the input. This is a necessary step to using debug records by default in all (other) LLVM tools. The implementation of this flag may not be to everyone's liking - setting it to "true" directly overwrites the value of the flags: --experimental-debuginfo-iterators The reason for this approach is that when we pass this new flag, we're intending to override any effect that those flags could actually have on the IR; this could be done by adding a check at every site where those flags are used, but I'm planning to rewrite or remove those flags soon, so this method avoids updating the same set of code twice without a functional change; if modifying the flag values is a problem though, then this can be rewritten. Patch is 24.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87379.diff 15 Files Affected:
diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index 8ebd0d3409c899..b4e971fea1a139 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -335,6 +335,7 @@ namespace llvm {
// Top-Level Entities
bool parseTopLevelEntities();
+ bool finalizeDebugInfoFormat(Module *M);
void dropUnknownMetadataReferences();
bool validateEndOfModule(bool UpgradeDebugInfo);
bool validateEndOfIndex();
diff --git a/llvm/include/llvm/IR/AutoUpgrade.h b/llvm/include/llvm/IR/AutoUpgrade.h
index 152f781ffa9b30..97c3e4d7589d7b 100644
--- a/llvm/include/llvm/IR/AutoUpgrade.h
+++ b/llvm/include/llvm/IR/AutoUpgrade.h
@@ -36,7 +36,8 @@ namespace llvm {
/// for upgrading, and returns true if it requires upgrading. It may return
/// null in NewFn if the all calls to the original intrinsic function
/// should be transformed to non-function-call instructions.
- bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn);
+ bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn,
+ bool CanUpgradeDebugIntrinsicsToRecords = true);
/// This is the complement to the above, replacing a specific call to an
/// intrinsic function with a call to the specified new function.
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index 0eea4cdccca5bb..f14d5d92da72de 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -81,17 +81,17 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
/// intrinsics into DbgMarkers / DbgRecords. Deletes all dbg.values in
/// the process and sets IsNewDbgInfoFormat = true. Only takes effect if
/// the UseNewDbgInfoFormat LLVM command line option is given.
- void convertToNewDbgValues();
+ void convertToNewDbgValues(bool UpdateFlagOnly = false);
/// Convert variable location debugging information stored in DbgMarkers and
/// DbgRecords into the dbg.value intrinsic representation. Sets
/// IsNewDbgInfoFormat = false.
- void convertFromNewDbgValues();
+ void convertFromNewDbgValues(bool UpdateFlagOnly = false);
/// Ensure the block is in "old" dbg.value format (\p NewFlag == false) or
/// in the new format (\p NewFlag == true), converting to the desired format
/// if necessary.
- void setIsNewDbgInfoFormat(bool NewFlag);
+ void setIsNewDbgInfoFormat(bool NewFlag, bool UpdateFlagOnly = false);
/// Record that the collection of DbgRecords in \p M "trails" after the last
/// instruction of this block. These are equivalent to dbg.value intrinsics
diff --git a/llvm/include/llvm/IR/Function.h b/llvm/include/llvm/IR/Function.h
index d96d506a9b05d0..8c881f38748fdc 100644
--- a/llvm/include/llvm/IR/Function.h
+++ b/llvm/include/llvm/IR/Function.h
@@ -114,12 +114,12 @@ class LLVM_EXTERNAL_VISIBILITY Function : public GlobalObject,
}
/// \see BasicBlock::convertToNewDbgValues.
- void convertToNewDbgValues();
+ void convertToNewDbgValues(bool UpdateFlagOnly = false);
/// \see BasicBlock::convertFromNewDbgValues.
- void convertFromNewDbgValues();
+ void convertFromNewDbgValues(bool UpdateFlagOnly = false);
- void setIsNewDbgInfoFormat(bool NewVal);
+ void setIsNewDbgInfoFormat(bool NewVal, bool UpdateFlagOnly = false);
private:
friend class TargetLibraryInfoImpl;
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index bb2e667ef6f410..bc3986140f0b9a 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -224,26 +224,26 @@ class LLVM_EXTERNAL_VISIBILITY Module {
void removeDebugIntrinsicDeclarations();
/// \see BasicBlock::convertToNewDbgValues.
- void convertToNewDbgValues() {
+ void convertToNewDbgValues(bool UpdateFlagOnly = false) {
for (auto &F : *this) {
- F.convertToNewDbgValues();
+ F.convertToNewDbgValues(UpdateFlagOnly);
}
IsNewDbgInfoFormat = true;
}
/// \see BasicBlock::convertFromNewDbgValues.
- void convertFromNewDbgValues() {
+ void convertFromNewDbgValues(bool UpdateFlagOnly = false) {
for (auto &F : *this) {
- F.convertFromNewDbgValues();
+ F.convertFromNewDbgValues(UpdateFlagOnly);
}
IsNewDbgInfoFormat = false;
}
- void setIsNewDbgInfoFormat(bool UseNewFormat) {
+ void setIsNewDbgInfoFormat(bool UseNewFormat, bool UpdateFlagOnly = false) {
if (UseNewFormat && !IsNewDbgInfoFormat)
- convertToNewDbgValues();
+ convertToNewDbgValues(UpdateFlagOnly);
else if (!UseNewFormat && IsNewDbgInfoFormat)
- convertFromNewDbgValues();
+ convertFromNewDbgValues(UpdateFlagOnly);
}
/// The Module constructor. Note that there is no default constructor. You
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index f0be021668afa7..70a6ef864f5656 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -63,6 +63,9 @@ static cl::opt<bool> AllowIncompleteIR(
"metadata will be dropped)"));
extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
+extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
+extern bool WriteNewDbgInfoFormatToBitcode;
+extern cl::opt<bool> WriteNewDbgInfoFormat;
static std::string getTypeString(Type *T) {
std::string Result;
@@ -71,12 +74,20 @@ static std::string getTypeString(Type *T) {
return Tmp.str();
}
-// Currently, we should always process modules in the old debug info format by
-// default regardless of the module's format in IR; convert it to the old format
-// here.
-bool finalizeDebugInfoFormat(Module *M) {
- if (M)
+// Whatever debug info format we parsed, we should convert to the expected debug
+// info format immediately afterwards.
+bool LLParser::finalizeDebugInfoFormat(Module *M) {
+ // We should have already returned an error if we observed both intrinsics and
+ // records in this IR.
+ assert(!(SeenNewDbgInfoFormat && SeenOldDbgInfoFormat) &&
+ "Mixed debug intrinsics/records seen without a parsing error?");
+ if (PreserveInputDbgFormat == cl::boolOrDefault::BOU_TRUE) {
+ UseNewDbgInfoFormat = SeenNewDbgInfoFormat;
+ WriteNewDbgInfoFormatToBitcode = SeenNewDbgInfoFormat;
+ WriteNewDbgInfoFormat = SeenNewDbgInfoFormat;
+ } else if (M) {
M->setIsNewDbgInfoFormat(false);
+ }
return false;
}
@@ -6507,10 +6518,10 @@ bool LLParser::parseBasicBlock(PerFunctionState &PFS) {
if (SeenOldDbgInfoFormat)
return error(Lex.getLoc(), "debug record should not appear in a module "
"containing debug info intrinsics");
+ if (!SeenNewDbgInfoFormat)
+ M->setIsNewDbgInfoFormat(true, true);
SeenNewDbgInfoFormat = true;
Lex.Lex();
- if (!M->IsNewDbgInfoFormat)
- M->convertToNewDbgValues();
DbgRecord *DR;
if (parseDebugRecord(DR, PFS))
@@ -7912,6 +7923,8 @@ bool LLParser::parseCall(Instruction *&Inst, PerFunctionState &PFS,
return error(CallLoc, "llvm.dbg intrinsic should not appear in a module "
"using non-intrinsic debug info");
}
+ if (!SeenOldDbgInfoFormat)
+ M->setIsNewDbgInfoFormat(false, true);
SeenOldDbgInfoFormat = true;
}
CI->setAttributes(PAL);
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 6ee93f17792b4f..5d92d8bed061b3 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -108,6 +108,9 @@ cl::opt<cl::boolOrDefault> LoadBitcodeIntoNewDbgInfoFormat(
"load-bitcode-into-experimental-debuginfo-iterators", cl::Hidden,
cl::desc("Load bitcode directly into the new debug info format (regardless "
"of input format)"));
+extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
+extern bool WriteNewDbgInfoFormatToBitcode;
+extern cl::opt<bool> WriteNewDbgInfoFormat;
namespace {
@@ -682,6 +685,11 @@ class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {
/// (e.g.) blockaddress forward references.
bool WillMaterializeAllForwardRefs = false;
+ /// Tracks whether we have seen debug intrinsics or records in this bitcode;
+ /// seeing both in a single module is currently a fatal error.
+ bool SeenDebugIntrinsic = false;
+ bool SeenDebugRecord = false;
+
bool StripDebugInfo = false;
TBAAVerifier TBAAVerifyHelper;
@@ -3774,7 +3782,11 @@ Error BitcodeReader::globalCleanup() {
for (Function &F : *TheModule) {
MDLoader->upgradeDebugIntrinsics(F);
Function *NewFn;
- if (UpgradeIntrinsicFunction(&F, NewFn))
+ // If PreserveInputDbgFormat=true, then we don't know whether we want
+ // intrinsics or records, and we won't perform any conversions in either
+ // case, so don't upgrade intrinsics to records.
+ if (UpgradeIntrinsicFunction(
+ &F, NewFn, PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE))
UpgradedIntrinsics[&F] = NewFn;
// Look for functions that rely on old function attribute behavior.
UpgradeFunctionAttributes(F);
@@ -4301,10 +4313,13 @@ Error BitcodeReader::parseModule(uint64_t ResumeBit,
bool ShouldLazyLoadMetadata,
ParserCallbacks Callbacks) {
// Load directly into RemoveDIs format if LoadBitcodeIntoNewDbgInfoFormat
- // has been set to true (default action: load into the old debug format).
- TheModule->IsNewDbgInfoFormat =
- UseNewDbgInfoFormat &&
- LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_TRUE;
+ // has been set to true and we aren't attempting to preserve the existing
+ // format in the bitcode (default action: load into the old debug format).
+ if (PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE) {
+ TheModule->IsNewDbgInfoFormat =
+ UseNewDbgInfoFormat &&
+ LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_TRUE;
+ }
this->ValueTypeCallback = std::move(Callbacks.ValueType);
if (ResumeBit) {
@@ -6443,6 +6458,7 @@ Error BitcodeReader::parseFunctionBody(Function *F) {
case bitc::FUNC_CODE_DEBUG_RECORD_ASSIGN: {
// DbgVariableRecords are placed after the Instructions that they are
// attached to.
+ SeenDebugRecord = true;
Instruction *Inst = getLastInstruction();
if (!Inst)
return error("Invalid dbg record: missing instruction");
@@ -6603,6 +6619,8 @@ Error BitcodeReader::parseFunctionBody(Function *F) {
TCK = CallInst::TCK_NoTail;
cast<CallInst>(I)->setTailCallKind(TCK);
cast<CallInst>(I)->setAttributes(PAL);
+ if (isa<DbgInfoIntrinsic>(I))
+ SeenDebugIntrinsic = true;
if (Error Err = propagateAttributeTypes(cast<CallBase>(I), ArgTyIDs)) {
I->deleteValue();
return Err;
@@ -6791,20 +6809,46 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
if (Error JumpFailed = Stream.JumpToBit(DFII->second))
return JumpFailed;
- // Set the debug info mode to "new", possibly creating a mismatch between
- // module and function debug modes. This is okay because we'll convert
- // everything back to the old mode after parsing if needed.
- // FIXME: Remove this once all tools support RemoveDIs.
+ // Regardless of the debug info format we want to end up in, we need
+ // IsNewDbgInfoFormat=true to construct any debug records seen in the bitcode.
F->IsNewDbgInfoFormat = true;
if (Error Err = parseFunctionBody(F))
return Err;
F->setIsMaterializable(false);
- // Convert new debug info records into intrinsics.
- // FIXME: Remove this once all tools support RemoveDIs.
- if (!F->getParent()->IsNewDbgInfoFormat)
- F->convertFromNewDbgValues();
+ // All parsed Functions should load into the debug info format dictated by the
+ // Module, unless we're attempting to preserve the input debug info format.
+ if (SeenDebugIntrinsic && SeenDebugRecord)
+ return error("Mixed debug intrinsics and debug records in bitcode module!");
+ if (PreserveInputDbgFormat == cl::boolOrDefault::BOU_TRUE) {
+ bool SeenAnyDebugInfo = SeenDebugIntrinsic || SeenDebugRecord;
+ bool NewDbgInfoFormatDesired =
+ SeenAnyDebugInfo ? SeenDebugRecord : F->getParent()->IsNewDbgInfoFormat;
+ if (SeenAnyDebugInfo) {
+ UseNewDbgInfoFormat = SeenDebugRecord;
+ WriteNewDbgInfoFormatToBitcode = SeenDebugRecord;
+ WriteNewDbgInfoFormat = SeenDebugRecord;
+ }
+ // If the module's debug info format doesn't match the observed input
+ // format, then set its format now; we don't need to call the conversion
+ // function because there must be no existing intrinsics to convert.
+ // Otherwise, just set the format on this function now.
+ if (NewDbgInfoFormatDesired != F->getParent()->IsNewDbgInfoFormat)
+ F->getParent()->setIsNewDbgInfoFormat(NewDbgInfoFormatDesired, true);
+ else
+ F->setIsNewDbgInfoFormat(NewDbgInfoFormatDesired, true);
+ } else {
+ // If we aren't preserving formats, we use the Module flag to get our
+ // desired format instead of reading flags, in case we are lazy-loading and
+ // the format of the module has been changed since it was set by the flags.
+ // We only need to convert debug info here if we have debug records but
+ // desire the intrinsic format; everything else is a no-op or handled by the
+ // autoupgrader.
+ bool ModuleIsNewDbgInfoFormat = F->getParent()->IsNewDbgInfoFormat;
+ F->setIsNewDbgInfoFormat(ModuleIsNewDbgInfoFormat,
+ ModuleIsNewDbgInfoFormat || !SeenDebugRecord);
+ }
if (StripDebugInfo)
stripDebugInfo(*F);
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index a44f6af4162f03..dc0b7347e4fe4e 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -983,7 +983,8 @@ static Intrinsic::ID shouldUpgradeNVPTXBF16Intrinsic(StringRef Name) {
return Intrinsic::not_intrinsic;
}
-static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
+static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn,
+ bool CanUpgradeDebugIntrinsicsToRecords) {
assert(F && "Illegal to upgrade a non-existent Function.");
StringRef Name = F->getName();
@@ -1057,7 +1058,8 @@ static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
case 'd':
if (Name.consume_front("dbg.")) {
// Mark debug intrinsics for upgrade to new debug format.
- if (F->getParent()->IsNewDbgInfoFormat) {
+ if (CanUpgradeDebugIntrinsicsToRecords &&
+ F->getParent()->IsNewDbgInfoFormat) {
if (Name == "addr" || Name == "value" || Name == "assign" ||
Name == "declare" || Name == "label") {
// There's no function to replace these with.
@@ -1413,9 +1415,11 @@ static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
return false;
}
-bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn) {
+bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn,
+ bool CanUpgradeDebugIntrinsicsToRecords) {
NewFn = nullptr;
- bool Upgraded = upgradeIntrinsicFunction1(F, NewFn);
+ bool Upgraded =
+ upgradeIntrinsicFunction1(F, NewFn, CanUpgradeDebugIntrinsicsToRecords);
assert(F != NewFn && "Intrinsic function upgraded to the same function");
// Upgrade intrinsic attributes. This does not change the function.
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index f088c7a2cc4e77..3bb0f423f270ca 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -35,6 +35,12 @@ cl::opt<bool>
cl::desc("Enable communicating debuginfo positions "
"through iterators, eliminating intrinsics"),
cl::init(true));
+cl::opt<cl::boolOrDefault> PreserveInputDbgFormat(
+ "preserve-input-debuginfo-format", cl::Hidden,
+ cl::desc("When set to true, IR files will be processed and printed in "
+ "their current debug info format, regardless of default behaviour "
+ "or other flags passed. Has no effect if input IR does not "
+ "contain debug records or intrinsics."));
bool WriteNewDbgInfoFormatToBitcode /*set default value in cl::init() below*/;
cl::opt<bool, true> WriteNewDbgInfoFormatToBitcode2(
@@ -65,8 +71,10 @@ DbgMarker *BasicBlock::createMarker(InstListType::iterator It) {
return DM;
}
-void BasicBlock::convertToNewDbgValues() {
+void BasicBlock::convertToNewDbgValues(bool UpdateFlagOnly) {
IsNewDbgInfoFormat = true;
+ if (UpdateFlagOnly)
+ return;
// Iterate over all instructions in the instruction list, collecting debug
// info intrinsics and converting them to DbgRecords. Once we find a "real"
@@ -104,9 +112,11 @@ void BasicBlock::convertToNewDbgValues() {
}
}
-void BasicBlock::convertFromNewDbgValues() {
- invalidateOrders();
+void BasicBlock::convertFromNewDbgValues(bool UpdateFlagOnly) {
IsNewDbgInfoFormat = false;
+ if (UpdateFlagOnly)
+ return;
+ invalidateOrders();
// Iterate over the block, finding instructions annotated with DbgMarkers.
// Convert any attached DbgRecords to debug intrinsics and insert ahead of the
@@ -141,11 +151,11 @@ void BasicBlock::dumpDbgValues() const {
}
#endif
-void BasicBlock::setIsNewDbgInfoFormat(bool NewFlag) {
+void BasicBlock::setIsNewDbgInfoFormat(bool NewFlag, bool UpdateFlagOnly) {
if (NewFlag && !IsNewDbgInfoFormat)
- convertToNewDbgValues();
+ convertToNewDbgValues(UpdateFlagOnly);
else if (!NewFlag && IsNewDbgInfoFormat)
- convertFromNewDbgValues();
+ convertFromNewDbgValues(UpdateFlagOnly);
}
ValueSymbolTable *BasicBlock::getValueSymbolTable() {
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index eb126f182eadcb..953977ca33ed40 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -83,25 +83,25 @@ static cl::opt<unsigned> NonGlobalValueMaxNameSize(
"non-global-value-max-name-size", cl::Hidden, cl::init(1024),
cl::desc("Maximum size for the name of non-global values."));
-void Function::convertToNewDbgValues() {
+void Function::convertToNewDbgValues(bool UpdateFlagOnly) {
IsNewDbgInfoFormat = true;
for (auto &BB : *this) {
- BB.convertToNewDbgValues();
+ BB.convertToNewDbgValues(UpdateFlagOnly);
}
}
-void Function::convertFromNewDbgValues() {
+void Function::convertFromNewDbgValues(bool UpdateFlagOnly) {
IsNewDbgInfoFormat = false;
for (auto &BB : *this) {
- BB.convertFromNewDbgValues();
+ BB.convertFromNewDbgValues(UpdateFlagOnly);
}
}
-void Function::setIsNewDbgInfoFormat(bool NewFlag) {
+void Function::setIsNewDbgInfoFormat(bool NewFlag, bool UpdateFlagOnly) {
if (NewFlag && !IsNewDbgInfoFormat)
- convertToNewDbgValues();
+ convertToNewDbgValues(UpdateFlagOnly);
else if (!NewFlag && IsNewDbgInfoFormat)
- convertFromNewDbgValues();
+ convertFromNewDbgValues(UpdateFlagOnly);
}
//===----------------------------------------------------------------------===//
diff --git a/llvm/test/Bitcode/dbg-record-roundtrip.ll b/llvm/test/Bitcode/dbg-record-roundtrip.ll
index bd347cac720677..cc83fdd4fa5381 100644
--- a/llvm/test/Bitcode/dbg-record-roundtrip.ll
+++ b/llvm/test/Bitcode/dbg-record-roundtrip.ll
@@ -15,6 +15,16 @@
; RUN: | llvm-dis --load-bitcode-into-experimental-debuginfo-iterators=true --write-experimental-debuginfo=true \
; RUN: | FileCheck %s --check-prefixes=RECORDS
+;; When preserving, we should output the format the bitcode was written in
+;; regardless of the value of the write flag.
+; RUN: llvm-as --write-experimental-debuginfo-iterators-to-bitcode=true %s -o - \
+; RUN: | llvm-dis --preserve-input-debuginfo-format=true --write-experimental-debuginfo=false \
+; RUN: | FileCheck %s --check-prefixes=RECORDS
+
+; RUN: llvm-as --write-experimental-debuginfo-iterators-to-bitcode=false %s -o - \
+; RUN: | llvm-dis --preserve-input-debuginfo-format=true --write-experimental-debuginfo=true \
+; RUN: | FileCheck %s
+
;; Check that verify-uselistorder passes regardless of input format.
; RUN: llvm-as %s --write-experimental-debuginfo-iterators-to-bitcode=true -o - | verify-uselistorder
; RUN: verify-uselistorder %s
diff --git a/llvm/test/DebugInfo/round...
[truncated]
|
OCHyams
reviewed
Apr 3, 2024
There was a problem hiding this comment.
I agree this functionality is needed. Meddling with the other flags so far from initialisation feels a little sketchy - I feel like it could easily become confusing and/or buggy.
We discussed offline the possibility of a --experimental-debuginfo-iterators=[always|never|input] flag, and that might involve more invasive checks all over the place. Are there any other alternatives?
e.g. I wonder if this should be a flag at all. Would it make sense for this to just be a (non-flag) global that llvm-reduce and llvm-verifyuselistorder opt into unconditionally?
| @@ -36,7 +36,8 @@ namespace llvm { | |||
| /// for upgrading, and returns true if it requires upgrading. It may return | |||
| /// null in NewFn if the all calls to the original intrinsic function | |||
| /// should be transformed to non-function-call instructions. | |||
| bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn); | |||
| bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn, | |||
| bool CanUpgradeDebugIntrinsicsToRecords = true); | |||
There was a problem hiding this comment.
Leaked in from another pull request?
There was a problem hiding this comment.
Nope, this part is used in this patch in BitcodeReader.cpp:3789, to avoid upgrading debug intrinsics to records specifically when we have the preserve flag enabled.
llvm/include/llvm/IR/BasicBlock.h
Outdated
| @@ -81,17 +81,17 @@ class BasicBlock final : public Value, // Basic blocks are data objects also | |||
| /// intrinsics into DbgMarkers / DbgRecords. Deletes all dbg.values in | |||
| /// the process and sets IsNewDbgInfoFormat = true. Only takes effect if | |||
| /// the UseNewDbgInfoFormat LLVM command line option is given. | |||
| void convertToNewDbgValues(); | |||
| void convertToNewDbgValues(bool UpdateFlagOnly = false); | |||
There was a problem hiding this comment.
This is a bit of a nit, but something about having the additional UpdateFlagOnly param in these (and the other similar) functions feels slightly off. I think a separate function would signal intent more clearly that only a flag is changing. YMMV. If you prefer to keep the new parameter, please can you add a doc-comment describing its purpose?
This "offness" might be solved by simply adding inline comments to the call sites , e.g. convertToNewDbgValues(true, /*UpdateFlagOnly=*/true).
wdyt?
There was a problem hiding this comment.
I think a separate function would signal intent more clearly that only a flag is changing.
This is probably fine - one of the reasons for making it a parameter rather than a function was that in one of the cases here, we're using a non-constant as the argument here (i.e. /*UpdateFlagOnly=*/ ModuleIsNewDbgInfoFormat || !SeenDebugRecord); this can easily be changed to an if-else though.
llvm/lib/IR/BasicBlock.cpp
Outdated
| cl::desc("When set to true, IR files will be processed and printed in " | ||
| "their current debug info format, regardless of default behaviour " | ||
| "or other flags passed. Has no effect if input IR does not " | ||
| "contain debug records or intrinsics.")); |
There was a problem hiding this comment.
"ignored in llvm-link, llvm-lto, llvm-lto2"
Main reason this is a flag rather than just an internal global is that it should be user controllable - for llvm-reduce at least it's generally desirable to maintain format, but may not always be intended. Aside from that, it will be a flag-controllable feature later on (as you mentioned), so it seems sensible to introduce it as such now. |
OCHyams
reviewed
Apr 4, 2024
There was a problem hiding this comment.
Aside from that, it will be a flag-controllable feature later on (as you mentioned), so it seems sensible to introduce it as such now.
If we're definitely moving in that direction, towards a unified flag, then I think this could be an acceptable stepping stone.
Can you add comments (and update any flag description text if there is any) for the other flags explaining that they get overridden by this one?
| LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_TRUE; | ||
| // has been set to true and we aren't attempting to preserve the existing | ||
| // format in the bitcode (default action: load into the old debug format). | ||
| if (PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE) { |
There was a problem hiding this comment.
Why isn't this == cl::boolOrDefault::BOU_TRUE?
There was a problem hiding this comment.
The intent is that if we're preserving the input format, then we don't want to update TheModule->IsNewDbgInfoFormat according to the rule below. It could all be folded into a boolean expression, but I think that might obfuscate the meaning.
There was a problem hiding this comment.
Ah I think I see where my confusion comes in, TheModule->IsNewDbgInfoFormat is currently always false. Is this patch intended to land after the "set IsNewDbgInfoFormat in Module ctor based on UseNewDbgInfoFormat" patch (alternatively: is your reply above intended to be true after that patch lands, even if this one lands first)?
Otherwise, without the module-ctor-patch, the default behaviour becomes convert all inputs into intrinsic mode (because of the code down on line 6842). Or am I misreading this situation?
There was a problem hiding this comment.
This doesn't require the ctor change; the behaviour is that:
- If we aren't preserving inputs, then we follow the existing behaviour - we set
IsNewDbgInfoFormataccording to theUseNewDbgInfoFormatandLoadBitcodeIntoNewDbgInfoFormatflags, and then when we materialize the function we either 1) convert any existing records to intrinsics via the call tosetIsNewDbgInfoFormaton line 6849, or 2) convert any existing intrinsics to records during autoupgrading, depending on whether we're loading into the new format or not. - If we are preserving inputs, then we never convert between intrinsics and records at any point - we insert records/intrinsics as we materialize the function, and then we set the
IsNewDbgInfoFormatflag on the module and its functions once we've determined what format the input was in.
OCHyams
approved these changes
Apr 5, 2024
|
Hello @SLTozer It results in |
|
I am seeing a similar crash when building the Linux kernel for arm64 after this change, although I only see it when LTO is enabled, so it may or may not be the exact same problem as above? struct hc_driver {
int (*urb_dequeue)();
};
static int isp1760_hc_setup() {
unsigned val = ({
({
for (;;)
;
val;
});
});
}
void isp1760_irq() {
goto leave;
leave:;
}
struct hc_driver isp1760_hc_driver = {isp1760_hc_setup};If this is not going to be forward fixed quickly, can the change be reverted in the meantime? |
The PR author and several of the reviewers here have been attending EuroLLVM this week, and so they may not have noticed updates here. I assume they will reply early next week, but if action is needed before then, I suggest reverting. |
SLTozer
added a commit
that referenced
this pull request
Apr 15, 2024
…88718) Fixes the reported errors on: #87379 A previous patch updated the bitcode reading for debug intrinsics/records to not perform the expensive debug info format conversion from records to intrinsics in cases where no records were present, but the patch did not actually track when debug labels had been seen, resulting in errors when parsing bitcode where functions contained debug label records but no other debug records. This patch fixes that case and adds a test for it.
|
Apologies for the delay, as mentioned above I was away for the last week. The error in this patch turned out to be fairly obvious, and should be fixed by #88718, which I've just merged. |
|
Thanks! The fix solves the problem we saw. |
bazuzi
pushed a commit
to bazuzi/llvm-project
that referenced
this pull request
Apr 15, 2024
…lvm#88718) Fixes the reported errors on: llvm#87379 A previous patch updated the bitcode reading for debug intrinsics/records to not perform the expensive debug info format conversion from records to intrinsics in cases where no records were present, but the patch did not actually track when debug labels had been seen, resulting in errors when parsing bitcode where functions contained debug label records but no other debug records. This patch fixes that case and adds a test for it.
aniplcc
pushed a commit
to aniplcc/llvm-project
that referenced
this pull request
Apr 15, 2024
…lvm#88718) Fixes the reported errors on: llvm#87379 A previous patch updated the bitcode reading for debug intrinsics/records to not perform the expensive debug info format conversion from records to intrinsics in cases where no records were present, but the patch did not actually track when debug labels had been seen, resulting in errors when parsing bitcode where functions contained debug label records but no other debug records. This patch fixes that case and adds a test for it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds a new flag:
--preserve-input-debuginfo-formatThis flag instructs the tool to not convert the debug info format (intrinsics/records) of input IR, but to instead determine the format of the input IR and overwrite the other format-determining flags so that we process and output the file in the same format that we received it in. This flag is turned off by llvm-link, llvm-lto, and llvm-lto2, and should be turned off by any other tool that expects to parse multiple IR modules and have their debug info formats match.
The motivation for this flag is to allow tools to not convert the debug info format - verify-uselistorder and llvm-reduce, and any downstream tools that seek to test or mutate IR as-is, without applying extraneous modifications to the input. This is a necessary step to using debug records by default in all (other) LLVM tools.
The implementation of this flag may not be to everyone's liking - setting it to "true" directly overwrites the value of the flags:
--experimental-debuginfo-iterators
--write-experimental-debuginfo
--write-experimental-debuginfo-iterators-to-bitcode
The reason for this approach is that when we pass this new flag, we're intending to override any effect that those flags could actually have on the IR; this could be done by adding a check at every site where those flags are used, but I'm planning to rewrite or remove those flags soon, so this method avoids updating the same set of code twice without a functional change; if modifying the flag values is a problem though, then this can be rewritten.