From 7396e4aedce0e96c75e5fe56f4ddc1383c8af125 Mon Sep 17 00:00:00 2001 From: Daniel Bertalan Date: Tue, 27 Jul 2021 11:03:24 +0200 Subject: [PATCH] LibDebug: Store 64-bit numbers in AttributeValue This helps us avoid weird truncation issues and fixes a bug on Clang builds where truncation while reading caused the DIE offsets following large LEB128 numbers to be incorrect. This removes the need for the separate `LongUnsignedNumber` type. --- Tests/AK/TestSourceLocation.cpp | 7 +++-- Userland/Libraries/LibDebug/DebugInfo.cpp | 16 +++++----- .../Libraries/LibDebug/Dwarf/AttributeValue.h | 6 ++-- Userland/Libraries/LibDebug/Dwarf/DIE.cpp | 2 +- .../Libraries/LibDebug/Dwarf/DwarfInfo.cpp | 30 +++++++++---------- .../Libraries/LibDebug/Dwarf/LineProgram.cpp | 2 +- 6 files changed, 32 insertions(+), 31 deletions(-) diff --git a/Tests/AK/TestSourceLocation.cpp b/Tests/AK/TestSourceLocation.cpp index 8f79f1d104a..f41e5674e30 100644 --- a/Tests/AK/TestSourceLocation.cpp +++ b/Tests/AK/TestSourceLocation.cpp @@ -13,9 +13,12 @@ TEST_CASE(basic_scenario) { auto location = SourceLocation::current(); - EXPECT_EQ(StringView(__FILE__), location.filename()); EXPECT_EQ(StringView(__FUNCTION__), location.function_name()); - EXPECT_EQ(__LINE__ - 3u, location.line_number()); + EXPECT_EQ(__LINE__ - 2u, location.line_number()); + // FIXME: On Clang, __FILE__ is a relative path, while location.path() is absolute +#ifndef __clang__ + EXPECT_EQ(StringView(__FILE__), location.filename()); +#endif } static StringView test_default_arg(const SourceLocation& loc = SourceLocation::current()) diff --git a/Userland/Libraries/LibDebug/DebugInfo.cpp b/Userland/Libraries/LibDebug/DebugInfo.cpp index 35becb1e213..fc54a5d82ef 100644 --- a/Userland/Libraries/LibDebug/DebugInfo.cpp +++ b/Userland/Libraries/LibDebug/DebugInfo.cpp @@ -193,7 +193,7 @@ static Optional parse_variable_type_die(Dwarf::DIE const& variable_d VERIFY(type_die_offset.value().type == Dwarf::AttributeValue::Type::DieReference); - auto type_die = variable_die.compilation_unit().get_die_at_offset(type_die_offset.value().data.as_u32); + auto type_die = variable_die.compilation_unit().get_die_at_offset(type_die_offset.value().data.as_unsigned); auto type_name = type_die.get_attribute(Dwarf::Attribute::Name); if (type_name.has_value()) { variable_info.type_name = type_name.value().data.as_string; @@ -258,10 +258,10 @@ OwnPtr DebugInfo::create_variable_info(Dwarf::DIE const VERIFY(constant.has_value()); switch (constant.value().type) { case Dwarf::AttributeValue::Type::UnsignedNumber: - variable_info->constant_data.as_u32 = constant.value().data.as_u32; + variable_info->constant_data.as_u32 = constant.value().data.as_unsigned; break; case Dwarf::AttributeValue::Type::SignedNumber: - variable_info->constant_data.as_i32 = constant.value().data.as_i32; + variable_info->constant_data.as_i32 = constant.value().data.as_signed; break; case Dwarf::AttributeValue::Type::String: variable_info->constant_data.as_string = constant.value().data.as_string; @@ -298,7 +298,7 @@ void DebugInfo::add_type_info_to_variable(Dwarf::DIE const& type_die, PtraceRegi if (is_array_type && member.tag() == Dwarf::EntryTag::SubRangeType) { auto upper_bound = member.get_attribute(Dwarf::Attribute::UpperBound); VERIFY(upper_bound.has_value()); - auto size = upper_bound.value().data.as_u32 + 1; + auto size = upper_bound.value().data.as_unsigned + 1; type_info->dimension_sizes.append(size); return; } @@ -433,11 +433,11 @@ Optional DebugInfo::get_source_path_of_inl u32 file_index = 0; if (caller_file->type == Dwarf::AttributeValue::Type::UnsignedNumber) { - file_index = caller_file->data.as_u32; + file_index = caller_file->data.as_unsigned; } else if (caller_file->type == Dwarf::AttributeValue::Type::SignedNumber) { // For some reason, the file_index is sometimes stored as a signed number. - VERIFY(caller_file->data.as_i32 >= 0); - file_index = (u32)caller_file->data.as_i32; + VERIFY(caller_file->data.as_signed >= 0); + file_index = (u32)caller_file->data.as_signed; } else { return {}; } @@ -455,7 +455,7 @@ Optional DebugInfo::get_line_of_inline(Dwarf::DIE const& die) const if (caller_line->type != Dwarf::AttributeValue::Type::UnsignedNumber) return {}; - return caller_line.value().data.as_u32; + return caller_line.value().data.as_unsigned; } } diff --git a/Userland/Libraries/LibDebug/Dwarf/AttributeValue.h b/Userland/Libraries/LibDebug/Dwarf/AttributeValue.h index 258f60e55ba..ec13f0aec29 100644 --- a/Userland/Libraries/LibDebug/Dwarf/AttributeValue.h +++ b/Userland/Libraries/LibDebug/Dwarf/AttributeValue.h @@ -15,7 +15,6 @@ struct AttributeValue { enum class Type : u8 { UnsignedNumber, SignedNumber, - LongUnsignedNumber, String, DieReference, // Reference to another DIE in the same compilation unit Boolean, @@ -26,9 +25,8 @@ struct AttributeValue { union { FlatPtr as_addr; - u32 as_u32; - i32 as_i32; - u64 as_u64; + u64 as_unsigned; + i64 as_signed; const char* as_string; // points to bytes in the memory mapped elf image bool as_bool; struct { diff --git a/Userland/Libraries/LibDebug/Dwarf/DIE.cpp b/Userland/Libraries/LibDebug/Dwarf/DIE.cpp index e6f546640c5..66e15e56ab0 100644 --- a/Userland/Libraries/LibDebug/Dwarf/DIE.cpp +++ b/Userland/Libraries/LibDebug/Dwarf/DIE.cpp @@ -75,7 +75,7 @@ void DIE::for_each_child(Function callback) const auto sibling = current_child->get_attribute(Attribute::Sibling); u32 sibling_offset = 0; if (sibling.has_value()) { - sibling_offset = sibling.value().data.as_u32; + sibling_offset = sibling.value().data.as_unsigned; } if (!sibling.has_value()) { diff --git a/Userland/Libraries/LibDebug/Dwarf/DwarfInfo.cpp b/Userland/Libraries/LibDebug/Dwarf/DwarfInfo.cpp index a2364b3d7e0..1195edaa9ea 100644 --- a/Userland/Libraries/LibDebug/Dwarf/DwarfInfo.cpp +++ b/Userland/Libraries/LibDebug/Dwarf/DwarfInfo.cpp @@ -90,7 +90,7 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im debug_info_stream >> data; VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::UnsignedNumber; - value.data.as_u32 = data; + value.data.as_unsigned = data; break; } case AttributeDataForm::Data2: { @@ -98,7 +98,7 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im debug_info_stream >> data; VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::UnsignedNumber; - value.data.as_u32 = data; + value.data.as_signed = data; break; } case AttributeDataForm::Addr: { @@ -110,19 +110,19 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im break; } case AttributeDataForm::SData: { - ssize_t data; + i64 data; debug_info_stream.read_LEB128_signed(data); VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::SignedNumber; - value.data.as_i32 = data; + value.data.as_signed = data; break; } case AttributeDataForm::UData: { - size_t data; + u64 data; debug_info_stream.read_LEB128_unsigned(data); VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::UnsignedNumber; - value.data.as_u32 = data; + value.data.as_unsigned = data; break; } case AttributeDataForm::SecOffset: { @@ -130,7 +130,7 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im debug_info_stream >> data; VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::SecOffset; - value.data.as_u32 = data; + value.data.as_unsigned = data; break; } case AttributeDataForm::Data4: { @@ -138,15 +138,15 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im debug_info_stream >> data; VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::UnsignedNumber; - value.data.as_u32 = data; + value.data.as_unsigned = data; break; } case AttributeDataForm::Data8: { u64 data; debug_info_stream >> data; VERIFY(!debug_info_stream.has_any_error()); - value.type = AttributeValue::Type::LongUnsignedNumber; - value.data.as_u64 = data; + value.type = AttributeValue::Type::UnsignedNumber; + value.data.as_unsigned = data; break; } case AttributeDataForm::Ref4: { @@ -155,7 +155,7 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im VERIFY(!debug_info_stream.has_any_error()); value.type = AttributeValue::Type::DieReference; VERIFY(unit); - value.data.as_u32 = data + unit->offset(); + value.data.as_unsigned = data + unit->offset(); break; } case AttributeDataForm::FlagPresent: { @@ -225,7 +225,7 @@ AttributeValue DwarfInfo::get_attribute_value(AttributeDataForm form, ssize_t im case AttributeDataForm::ImplicitConst: { /* Value is part of the abbreviation record. */ value.type = AttributeValue::Type::SignedNumber; - value.data.as_i32 = implicit_const_value; + value.data.as_signed = implicit_const_value; break; } default: @@ -250,7 +250,7 @@ void DwarfInfo::build_cached_dies() const if (!start.has_value() || !end.has_value()) return {}; - VERIFY(start->type == Dwarf::AttributeValue::Type::UnsignedNumber); + VERIFY(start->form == Dwarf::AttributeDataForm::Addr); // DW_AT_high_pc attribute can have different meanings depending on the attribute form. // (Dwarf version 5, section 2.17.2). @@ -259,9 +259,9 @@ void DwarfInfo::build_cached_dies() const if (end->form == Dwarf::AttributeDataForm::Addr) range_end = end->data.as_addr; else - range_end = start->data.as_u32 + end->data.as_u32; + range_end = start->data.as_unsigned + end->data.as_unsigned; - return { DIERange { start.value().data.as_u32, range_end } }; + return { DIERange { (FlatPtr)start.value().data.as_addr, range_end } }; }; // If we simply use a lambda, type deduction fails because it's used recursively. diff --git a/Userland/Libraries/LibDebug/Dwarf/LineProgram.cpp b/Userland/Libraries/LibDebug/Dwarf/LineProgram.cpp index 821d8506086..323abf0696a 100644 --- a/Userland/Libraries/LibDebug/Dwarf/LineProgram.cpp +++ b/Userland/Libraries/LibDebug/Dwarf/LineProgram.cpp @@ -63,7 +63,7 @@ void LineProgram::parse_path_entries(Function callback, entry.path = value.data.as_string; break; case ContentType::DirectoryIndex: - entry.directory_index = value.data.as_u32; + entry.directory_index = value.data.as_unsigned; break; default: dbgln_if(DWARF_DEBUG, "Unhandled path list attribute: {}", (int)format_description.type);