From 67dbf2f192792996738ff30cba7bb5747cb718f8 Mon Sep 17 00:00:00 2001 From: Philip Craig Date: Tue, 3 Sep 2024 12:27:53 +1000 Subject: [PATCH] read: improve handling of 0 for tombstones Where possible, ignore these instead of returning them or returning an error. This often isn't possible because 0 is a valid address, but we can handle it for `DW_LNE_set_address` in the middle of a line sequence, and for address pairs. --- src/read/aranges.rs | 2 ++ src/read/line.rs | 23 ++++++++++------- src/read/loclists.rs | 53 +++++++++------------------------------ src/read/rnglists.rs | 59 +++++++++++++++++--------------------------- 4 files changed, 50 insertions(+), 87 deletions(-) diff --git a/src/read/aranges.rs b/src/read/aranges.rs index c2aac914..2829f261 100644 --- a/src/read/aranges.rs +++ b/src/read/aranges.rs @@ -290,6 +290,8 @@ impl ArangeEntryIter { #[doc(hidden)] pub fn convert_raw(&self, mut entry: ArangeEntry) -> Result> { // Skip tombstone entries. + // DWARF specifies a tombstone value of -1, but many linkers use 0. + // However, 0 may be a valid address, so the caller must handle that case. let address_size = self.encoding.address_size; let tombstone_address = !0 >> (64 - self.encoding.address_size * 8); if entry.range.begin == tombstone_address { diff --git a/src/read/line.rs b/src/read/line.rs index ceaf3b3f..1e91b020 100644 --- a/src/read/line.rs +++ b/src/read/line.rs @@ -828,12 +828,16 @@ impl LineRow { } LineInstruction::SetAddress(address) => { + // If the address is a tombstone, then skip instructions until the next address. + // DWARF specifies a tombstone value of -1, but many linkers use 0. + // However, 0 may be a valid address, so we only skip that if we have previously + // seen a higher address. Additionally, gold may keep the relocation addend, + // so we treat all lower addresses as tombstones instead of just 0. + // This works because DWARF specifies that addresses are monotonically increasing + // within a sequence; the alternative is to return an error. let tombstone_address = !0 >> (64 - program.header().encoding.address_size * 8); - self.tombstone = address == tombstone_address; + self.tombstone = address < self.address || address == tombstone_address; if !self.tombstone { - if address < self.address { - return Err(Error::InvalidAddressRange); - } self.address = address; self.op_index.0 = 0; } @@ -2877,13 +2881,14 @@ mod tests { #[test] fn test_exec_set_address_backwards() { let header = make_test_header(EndianSlice::new(&[], LittleEndian)); - let mut registers = LineRow::new(&header); - registers.address = 1; + let mut initial_registers = LineRow::new(&header); + initial_registers.address = 1; let opcode = LineInstruction::SetAddress(0); - let mut program = IncompleteLineProgram { header }; - let result = registers.execute(opcode, &mut program); - assert_eq!(result, Err(Error::InvalidAddressRange)); + let mut expected_registers = initial_registers; + expected_registers.tombstone = true; + + assert_exec_opcode(header, initial_registers, opcode, expected_registers, false); } #[test] diff --git a/src/read/loclists.rs b/src/read/loclists.rs index a2874687..ba88c10d 100644 --- a/src/read/loclists.rs +++ b/src/read/loclists.rs @@ -633,6 +633,7 @@ impl LocListIter { ), RawLocListEntry::AddressOrOffsetPair { begin, end, data } | RawLocListEntry::OffsetPair { begin, end, data } => { + // Skip tombstone entries (see below). if self.base_address == tombstone { return Ok(None); } @@ -651,7 +652,17 @@ impl LocListIter { } }; - if range.begin == tombstone || range.begin > range.end { + // Skip tombstone entries. + // + // DWARF specifies a tombstone value of -1 or -2, but many linkers use 0 or 1. + // However, 0/1 may be a valid address, so we can't always reliably skip them. + // One case where we can skip them is for address pairs, where both values are + // replaced by tombstones and thus `begin` equals `end`. Since these entries + // are empty, it's safe to skip them even if they aren't tombstones. + // + // In addition to skipping tombstone entries, we also skip invalid entries + // where `begin` is greater than `end`. This can occur due to compiler bugs. + if range.begin == tombstone || range.begin >= range.end { return Ok(None); } @@ -806,16 +817,6 @@ mod tests { ); // An empty location range followed by a normal location. - assert_eq!( - locations.next(), - Ok(Some(LocationListEntry { - range: Range { - begin: 0x0201_0600, - end: 0x0201_0600, - }, - data: Expression(EndianSlice::new(&[4, 0, 0, 0], LittleEndian)), - })) - ); assert_eq!( locations.next(), Ok(Some(LocationListEntry { @@ -1070,16 +1071,6 @@ mod tests { ); // An empty location range followed by a normal location. - assert_eq!( - locations.next(), - Ok(Some(LocationListEntry { - range: Range { - begin: 0x0201_0600, - end: 0x0201_0600, - }, - data: Expression(EndianSlice::new(&[4, 0, 0, 0], LittleEndian)), - })) - ); assert_eq!( locations.next(), Ok(Some(LocationListEntry { @@ -1290,16 +1281,6 @@ mod tests { ); // An empty location range followed by a normal location. - assert_eq!( - locations.next(), - Ok(Some(LocationListEntry { - range: Range { - begin: 0x0201_0600, - end: 0x0201_0600, - }, - data: Expression(EndianSlice::new(&[4, 0, 0, 0], LittleEndian)), - })) - ); assert_eq!( locations.next(), Ok(Some(LocationListEntry { @@ -1426,16 +1407,6 @@ mod tests { ); // An empty location range followed by a normal location. - assert_eq!( - locations.next(), - Ok(Some(LocationListEntry { - range: Range { - begin: 0x0201_0600, - end: 0x0201_0600, - }, - data: Expression(EndianSlice::new(&[4, 0, 0, 0], LittleEndian)), - })) - ); assert_eq!( locations.next(), Ok(Some(LocationListEntry { diff --git a/src/read/rnglists.rs b/src/read/rnglists.rs index 47d2a44b..5dddbe1e 100644 --- a/src/read/rnglists.rs +++ b/src/read/rnglists.rs @@ -556,6 +556,7 @@ impl RngListIter { } RawRngListEntry::AddressOrOffsetPair { begin, end } | RawRngListEntry::OffsetPair { begin, end } => { + // Skip tombstone entries (see below). if self.base_address == tombstone { return Ok(None); } @@ -570,7 +571,17 @@ impl RngListIter { } }; - if range.begin == tombstone || range.begin > range.end { + // Skip tombstone entries. + // + // DWARF specifies a tombstone value of -1 or -2, but many linkers use 0 or 1. + // However, 0/1 may be a valid address, so we can't always reliably skip them. + // One case where we can skip them is for address pairs, where both values are + // replaced by tombstones and thus `begin` equals `end`. Since these entries + // are empty, it's safe to skip them even if they aren't tombstones. + // + // In addition to skipping tombstone entries, we also skip invalid entries + // where `begin` is greater than `end`. This can occur due to compiler bugs. + if range.begin == tombstone || range.begin >= range.end { return Ok(None); } @@ -697,8 +708,9 @@ mod tests { .L8(7).L32(0x201_0c00).uleb(0x100) // An OffsetPair that starts at 0. .L8(4).uleb(0).uleb(1) - // An OffsetPair that starts and ends at 0. + // An OffsetPair that starts and ends at 0 followed by a normal OffsetPair. .L8(4).uleb(0).uleb(0) + .L8(4).uleb(0x10e00).uleb(0x10f00) // An OffsetPair that ends at -1. .L8(5).L32(0) .L8(4).uleb(0).uleb(0xffff_ffff) @@ -762,13 +774,6 @@ mod tests { ); // An empty range followed by a normal range. - assert_eq!( - ranges.next(), - Ok(Some(Range { - begin: 0x0201_0600, - end: 0x0201_0600, - })) - ); assert_eq!( ranges.next(), Ok(Some(Range { @@ -804,12 +809,12 @@ mod tests { })) ); - // A range that starts and ends at 0. + // A range that starts and ends at 0 followed by a normal range. assert_eq!( ranges.next(), Ok(Some(Range { - begin: 0x0200_0000, - end: 0x0200_0000, + begin: 0x0201_0e00, + end: 0x0201_0f00, })) ); @@ -921,8 +926,9 @@ mod tests { .L8(7).L64(0x201_0c00).uleb(0x100) // An OffsetPair that starts at 0. .L8(4).uleb(0).uleb(1) - // An OffsetPair that starts and ends at 0. + // An OffsetPair that starts and ends at 0 followed by a normal OffsetPair. .L8(4).uleb(0).uleb(0) + .L8(4).uleb(0x10e00).uleb(0x10f00) // An OffsetPair that ends at -1. .L8(5).L64(0) .L8(4).uleb(0).uleb(0xffff_ffff) @@ -986,13 +992,6 @@ mod tests { ); // An empty range followed by a normal range. - assert_eq!( - ranges.next(), - Ok(Some(Range { - begin: 0x0201_0600, - end: 0x0201_0600, - })) - ); assert_eq!( ranges.next(), Ok(Some(Range { @@ -1028,12 +1027,12 @@ mod tests { })) ); - // A range that starts and ends at 0. + // A range that starts and ends at 0 followed by a normal range. assert_eq!( ranges.next(), Ok(Some(Range { - begin: 0x0200_0000, - end: 0x0200_0000, + begin: 0x0201_0e00, + end: 0x0201_0f00, })) ); @@ -1199,13 +1198,6 @@ mod tests { ); // An empty range followed by a normal range. - assert_eq!( - ranges.next(), - Ok(Some(Range { - begin: 0x0201_0600, - end: 0x0201_0600, - })) - ); assert_eq!( ranges.next(), Ok(Some(Range { @@ -1317,13 +1309,6 @@ mod tests { ); // An empty range followed by a normal range. - assert_eq!( - ranges.next(), - Ok(Some(Range { - begin: 0x0201_0600, - end: 0x0201_0600, - })) - ); assert_eq!( ranges.next(), Ok(Some(Range {