[RISCV][Disassemble] Ensure the comment stream of the disassembler is set. #125962
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: Francesco Petrogalli (fpetrogalli) ChangesFull diff: https://github.com/llvm/llvm-project/pull/125962.diff 3 Files Affected:
diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 901bfcf5fa54f4e..cb773d0f9589c46 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -131,6 +131,11 @@ class MCDisassembler {
/// MCDisassembler::SoftFail if the instruction was
/// disassemblable but invalid,
/// MCDisassembler::Fail if the instruction was invalid.
+ ///
+ /// Note: to avoid potential issues caused by the field
+ /// `MCDisassembler::CommentStream` being set nullptr (its default
+ /// value), an implementation of this method should make sure to set
+ /// `CommentStream = &CStream;`.
virtual DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
ArrayRef<uint8_t> Bytes, uint64_t Address,
raw_ostream &CStream) const = 0;
diff --git a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
index 6aa4b0e4fcb99da..0dfa17eb1346fdb 100644
--- a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
@@ -29,17 +29,21 @@ bool MCDisassembler::tryAddingSymbolicOperand(MCInst &Inst, int64_t Value,
uint64_t Address, bool IsBranch,
uint64_t Offset, uint64_t OpSize,
uint64_t InstSize) const {
- if (Symbolizer)
+ if (Symbolizer) {
+ assert(CommentStream && "CommentStream is not set.");
return Symbolizer->tryAddingSymbolicOperand(Inst, *CommentStream, Value,
Address, IsBranch, Offset,
OpSize, InstSize);
+ }
return false;
}
void MCDisassembler::tryAddingPcLoadReferenceComment(int64_t Value,
uint64_t Address) const {
- if (Symbolizer)
+ if (Symbolizer) {
+ assert(CommentStream && "CommentStream is not set.");
Symbolizer->tryAddingPcLoadReferenceComment(*CommentStream, Value, Address);
+ }
}
void MCDisassembler::setSymbolizer(std::unique_ptr<MCSymbolizer> Symzer) {
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 3ec465810b1d114..01648ec0cbe9ea9 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -789,6 +789,7 @@ DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
ArrayRef<uint8_t> Bytes,
uint64_t Address,
raw_ostream &CS) const {
+ CommentStream = &CS;
// It's a 16 bit instruction if bit 0 and 1 are not 0b11.
if ((Bytes[0] & 0b11) != 0b11)
return getInstruction16(MI, Size, Bytes, Address, CS);
|
|
@llvm/pr-subscribers-mc Author: Francesco Petrogalli (fpetrogalli) ChangesFull diff: https://github.com/llvm/llvm-project/pull/125962.diff 3 Files Affected:
diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 901bfcf5fa54f4e..cb773d0f9589c46 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -131,6 +131,11 @@ class MCDisassembler {
/// MCDisassembler::SoftFail if the instruction was
/// disassemblable but invalid,
/// MCDisassembler::Fail if the instruction was invalid.
+ ///
+ /// Note: to avoid potential issues caused by the field
+ /// `MCDisassembler::CommentStream` being set nullptr (its default
+ /// value), an implementation of this method should make sure to set
+ /// `CommentStream = &CStream;`.
virtual DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
ArrayRef<uint8_t> Bytes, uint64_t Address,
raw_ostream &CStream) const = 0;
diff --git a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
index 6aa4b0e4fcb99da..0dfa17eb1346fdb 100644
--- a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
@@ -29,17 +29,21 @@ bool MCDisassembler::tryAddingSymbolicOperand(MCInst &Inst, int64_t Value,
uint64_t Address, bool IsBranch,
uint64_t Offset, uint64_t OpSize,
uint64_t InstSize) const {
- if (Symbolizer)
+ if (Symbolizer) {
+ assert(CommentStream && "CommentStream is not set.");
return Symbolizer->tryAddingSymbolicOperand(Inst, *CommentStream, Value,
Address, IsBranch, Offset,
OpSize, InstSize);
+ }
return false;
}
void MCDisassembler::tryAddingPcLoadReferenceComment(int64_t Value,
uint64_t Address) const {
- if (Symbolizer)
+ if (Symbolizer) {
+ assert(CommentStream && "CommentStream is not set.");
Symbolizer->tryAddingPcLoadReferenceComment(*CommentStream, Value, Address);
+ }
}
void MCDisassembler::setSymbolizer(std::unique_ptr<MCSymbolizer> Symzer) {
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 3ec465810b1d114..01648ec0cbe9ea9 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -789,6 +789,7 @@ DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
ArrayRef<uint8_t> Bytes,
uint64_t Address,
raw_ostream &CS) const {
+ CommentStream = &CS;
// It's a 16 bit instruction if bit 0 and 1 are not 0b11.
if ((Bytes[0] & 0b11) != 0b11)
return getInstruction16(MI, Size, Bytes, Address, CS);
|
fpetrogalli
force-pushed
the
ensure-disassembler-has-comment-stream
branch
from
4e1f72a to
91a73ec
Compare
|
@fpetrogalli not all disassemblers set CommentStream. This causes disassemblers to assert, including in lldb. I don't think there should be asserts in MCDisassembler.cpp |
|
Maybe all disassembler should set CommentStream for their target? Both functions are dereferencing assert(CommentStream && "CommentStream is not set.");
return Symbolizer->tryAddingSymbolicOperand(Inst, *CommentStream, Value,
Address, IsBranch, Offset,
OpSize, InstSize);
} |
|
There used to be code to handle a nullptr there, but it was removed in 1ea5ce6 |
|
Maybe we just need to set the comment stream in HexagonDisassembler::getInstruction? |
I'm talking to our guys internally about that; but there are other cores out there that will need to do the same thing. Plus, I don't like the idea of library code asserting on things. This is fine for something that is OK to die on a fatal error, but the debugger should never die on bad input. Just throw an error, and let the user continue. |
Is this the case? I don't think there is anything wrong with the input being debugged. I would consider this a bug of the debugger (assuming of course we decide to keep 1ea5ce6). |
The problem is when lldb instantiates the disassembler, if the target MCDisassembler code doesn't set CommentStream, it will be nullptr and the assert will fire, which takes down lldb. lldb doesn't control the CommentStream, but it not being set will cause lldb to crash. Maybe all the MCDisassemblers should set CommentStream, but that wasn't required in the past, so many don't. Debuggers inherently deal with bad input all the time, and shouldn't die when that happens, but return an error to the user. |
Is Hexagon just getting lucky that the null comment stream was never written to by the Symbolizer which would cause segmentation fault. |
Sets CommentStream after assert added in llvm#125962.
Sets CommentStream after assert added in #125962.
I'm not sure. I haven't worked in the guts of the disassembler in many years! It looks like we pushed up a change to add CommentStream to the Hexagon disassembler; thanks @quic-areg ! |
Sets CommentStream after assert added in llvm#125962.
Sets CommentStream after assert added in llvm#125962.
Sets CommentStream after assert added in llvm#125962.
Sets CommentStream after assert added in llvm#125962.
No description provided.