From: Petr Machata Date: Sat, 9 Oct 2010 18:45:04 +0000 (+0200) Subject: dwarflint: Streamlining code X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b1a22a783005e48861f42089ff1179dbc5adcd85;p=thirdparty%2Felfutils.git dwarflint: Streamlining code - Extract checks common to direct and indirect forms to check_debug_abbrev::check_form. - Reading of indirect form is now done via a new function read_sc_value in check_debug_info.cc. That function is also called for reading normal values of classes sc_value and sc_block --- diff --git a/dwarflint/check_debug_abbrev.cc b/dwarflint/check_debug_abbrev.cc index 4d0c6dac5..fac1f4688 100644 --- a/dwarflint/check_debug_abbrev.cc +++ b/dwarflint/check_debug_abbrev.cc @@ -111,13 +111,14 @@ namespace }; void - complain_invalid_form (where const &where, int name, int form, - std::string const &specification = "") + complain (where const *where, + attribute const *attribute, form const *form, + bool indirect, char const *qualifier) { - wr_error (where) - << specification << (" "[specification.length () == 0]) - << pri::attr (name) << " with invalid form " - << pri::form (form) << '.' << std::endl; + wr_error (*where) + << "attribute " << *attribute << " with " << qualifier + << (indirect ? " indirect" : "") << " form" + << *form << '.' << std::endl; } bool @@ -360,61 +361,53 @@ namespace /* Now if both are zero, this was the last attribute. */ null_attrib = attrib_name == 0 && attrib_form == 0; - attribute const *attribute = NULL; - form const *form = NULL; - if (!null_attrib) - { - /* Otherwise validate name and form. */ - attribute = ver->get_attribute (attrib_name); - if (attribute == NULL) - { - wr_error (where) - << "invalid name " << pri::hex (attrib_name) - << '.' << std::endl; - failed = true; - continue; - } - form = ver->get_form (attrib_form); - if (form == NULL) - { - wr_error (where) - << "invalid form " << pri::hex (attrib_form) - << '.' << std::endl; - failed = true; - continue; - } + REALLOC (cur, attribs); - if (!ver->form_allowed (attribute->name (), form->name ())) - complain_invalid_form (where, attrib_name, attrib_form, - "attribute"); + struct abbrev_attrib *acur = cur->attribs + cur->size++; + WIPE (*acur); + acur->name = attrib_name; + acur->form = attrib_form; + acur->where = where; - std::pair::iterator, bool> inserted - = seen.insert (std::make_pair (attrib_name, attr_off)); - if (!inserted.second) - { - wr_error (where) - << "duplicate attribute " << pri::attr (attrib_name) - << " (first was at " << pri::hex (inserted.first->second) - << ")." << std::endl; - // I think we may allow such files for high-level - // consumption, so don't fail the check... - if (attrib_name == DW_AT_sibling) - // ... unless it's DW_AT_sibling. - failed = true; - } + if (null_attrib) + break; + + /* Otherwise validate name and form. */ + attribute = ver->get_attribute (attrib_name); + if (attribute == NULL) + { + wr_error (where) + << "invalid name " << pri::hex (attrib_name) + << '.' << std::endl; + failed = true; + continue; } - REALLOC (cur, attribs); + std::pair::iterator, bool> inserted + = seen.insert (std::make_pair (attrib_name, attr_off)); + if (!inserted.second) + { + wr_error (where) + << "duplicate attribute " << pri::attr (attrib_name) + << " (first was at " << pri::hex (inserted.first->second) + << ")." << std::endl; + // I think we may allow such files for high-level + // consumption, so don't fail the check... + if (attrib_name == DW_AT_sibling) + // ... unless it's DW_AT_sibling. + failed = true; + } - struct abbrev_attrib *acur = cur->attribs + cur->size++; - WIPE (*acur); + form const *form = check_debug_abbrev::check_form + (ver, attrib_form, attribute, &where, false); + if (form == NULL) + { + failed = true; + continue; + } - /* We do structural checking of sibling attribute, so make - sure our assumptions in actual DIE-loading code are - right. We expect form from reference class, but only - CU-local, not DW_FORM_ref_addr. */ if (attrib_name == DW_AT_sibling) { if (!cur->has_children) @@ -422,35 +415,13 @@ namespace cat (mc_die_rel, mc_acc_bloat, mc_impact_1)) << "excessive DW_AT_sibling attribute at childless abbrev." << std::endl; - - switch (sibling_form_suitable (ver, attrib_form)) - { - case sfs_long: - wr_message (where, cat (mc_die_rel, mc_impact_2)) - << "DW_AT_sibling attribute with form DW_FORM_ref_addr." - << std::endl; - break; - - case sfs_invalid: - wr_error (where) - << "DW_AT_sibling attribute with non-reference form " - << pri::form (attrib_form) << '.' << std::endl; - - case sfs_ok: - ; - }; } - - else if (attrib_name == DW_AT_ranges) + if (attrib_name == DW_AT_ranges) ranges = true; else if (attrib_name == DW_AT_low_pc) low_pc = true; else if (attrib_name == DW_AT_high_pc) high_pc = true; - - acur->name = attrib_name; - acur->form = attrib_form; - acur->where = where; } while (!null_attrib); @@ -492,6 +463,32 @@ check_debug_abbrev::check_debug_abbrev (checkstack &stack, dwarflint &lint) { } +form const * +check_debug_abbrev::check_form (dwarf_version const *ver, int form_name, + attribute const *attribute, where const *where, + bool indirect) +{ + form const *form = ver->get_form (form_name); + if (form == NULL) + { + wr_error (*where) + << "invalid form " << pri::hex (form_name) + << '.' << std::endl; + return NULL; + } + + if (!ver->form_allowed (attribute->name (), form_name)) + { + complain (where, attribute, form, indirect, "invalid"); + return NULL; + } + else if (attribute->name () == DW_AT_sibling + && sibling_form_suitable (ver, form_name) == sfs_long) + complain (where, attribute, form, indirect, "unsuitable"); + + return form; +} + check_debug_abbrev::~check_debug_abbrev () { // xxx So using new[]/delete[] would be nicer (delete ignores diff --git a/dwarflint/check_debug_abbrev.hh b/dwarflint/check_debug_abbrev.hh index 9d1d77824..bdc7f8c6a 100644 --- a/dwarflint/check_debug_abbrev.hh +++ b/dwarflint/check_debug_abbrev.hh @@ -29,6 +29,7 @@ #include "checks.hh" #include "sections.ii" #include "check_debug_info.ii" +#include "dwarf_version.ii" struct abbrev_attrib { @@ -83,6 +84,12 @@ public: abbrev_map const abbrevs; check_debug_abbrev (checkstack &stack, dwarflint &lint); + static form const *check_form (dwarf_version const *ver, + int form_name, + attribute const *attribute, + where const *where, + bool indirect); + ~check_debug_abbrev (); }; diff --git a/dwarflint/check_debug_info.cc b/dwarflint/check_debug_info.cc index 6736d6472..29afad640 100644 --- a/dwarflint/check_debug_info.cc +++ b/dwarflint/check_debug_info.cc @@ -495,6 +495,37 @@ namespace ref_record_add (&ctx->cu->decl_file_refs, value, ctx->where); } + /// Read value depending on the form width and storage class. + bool + read_sc_value (uint64_t *valuep, form const *form, cu const *cu, + read_ctx *ctx, where *wherep) + { + form_width_t width = form->width (cu); + switch (width) + { + case fw_0: + // Who knows, DW_FORM_flag_absent might turn up one day... + assert (form->name () == DW_FORM_flag_present); + *valuep = 1; + return true; + + case fw_1: + case fw_2: + case fw_4: + case fw_8: + return read_ctx_read_var (ctx, width, valuep); + + case fw_uleb: + case fw_sleb: + return checked_read_leb128 (ctx, width, valuep, + wherep, "attribute value"); + + case fw_unknown: + ; + } + UNREACHABLE; + } + /* Returns: -1 in case of error @@ -628,42 +659,13 @@ namespace if (ver->form_class (form, attribute) == cl_indirect) { uint64_t value; - if (!checked_read_uleb128 (ctx, &value, &where, - "indirect attribute form")) + if (!read_sc_value (&value, form, cu, ctx, &where)) return -1; form_name = value; - form = ver->get_form (form_name); - - // xxx Some of what's below is probably duplicated in - // check_debug_abbrev. Plus we really want to run the - // same checks for direct and indirect attributes. - // Consolidate. - if (!ver->form_allowed (form_name)) - { - wr_error (where) - << "invalid indirect form " << pri::hex (value) - << '.' << std::endl; - return -1; - } - - if (attribute->name () == DW_AT_sibling) - switch (sibling_form_suitable (ver, form_name)) - { - case sfs_long: - wr_message (where, cat (mc_die_rel, mc_impact_2)) - << "DW_AT_sibling attribute with (indirect) form " - "DW_FORM_ref_addr." << std::endl; - break; - - case sfs_invalid: - wr_error (where) - << "DW_AT_sibling attribute with non-reference " - "(indirect) form \"" << pri::form (value) - << "\"." << std::endl; - - case sfs_ok: - ; - } + form = check_debug_abbrev::check_form + (ver, form_name, attribute, &where, true); + if (form == NULL) + return -1; } dw_class cls = ver->form_class (form, attribute); @@ -682,9 +684,6 @@ namespace uint64_t ctx_offset = read_ctx_get_offset (ctx) + cu->head->offset; bool type_is_rel = file.ehdr.e_type == ET_REL; - /* Attribute value. */ - uint64_t value = 0; - /* Whether the value should be relocated first. Note that relocations are really required only in REL files, so missing relocations are not warned on even with @@ -800,75 +799,46 @@ namespace break; } - /* Read value depending on the form width and storage - class. */ - form_width_t width = form->width (cu); + /* Attribute value. */ + uint64_t value; storage_class_t storclass = form->storage_class (); - switch (storclass) + if (storclass == sc_string) { - case sc_string: if (!read_ctx_read_str (ctx)) goto cant_read; - break; + } + else + { + if (!read_sc_value (&value, form, cu, ctx, &where)) + { + // Note that for fw_uleb and fw_sleb we report the + // error the second time now. + cant_read: + wr_error (where) + << "can't read value of attribute " + << *attribute << '.' << std::endl; + return -1; + } - case sc_block: - case sc_value: - // Read the value, or the length field if it's a block - // form. - switch (width) + if (storclass == sc_block) { - case fw_0: - // who knows, DW_FORM_flag_absent might turn up one day - assert (form->name () == DW_FORM_flag_present); - value = 1; - break; - - case fw_1: - case fw_2: - case fw_4: - case fw_8: - if (!read_ctx_read_var (ctx, width, &value)) + // Read & validate the block body. + if (cls == cl_exprloc) { - cant_read: - wr_error (where) - << "can't read value of attribute " - << pri::attr (it->name) << '.' << std::endl; - return -1; + uint64_t expr_start + = cu->head->offset + read_ctx_get_offset (ctx); + if (!check_location_expression + (file, ctx, cu, expr_start, reloc, value, &where)) + return false; } - break; - - case fw_uleb: - case fw_sleb: - if (!checked_read_leb128 (ctx, width, &value, - &where, "attribute value")) - return -1; - break; + else + relocation_skip (reloc, + read_ctx_get_offset (ctx) + value, + &where, skip_mismatched); - case fw_unknown: - assert (!"Should never get there!"); + if (!read_ctx_skip (ctx, value)) + goto cant_read; } - - if (storclass != sc_block) - break; - - // Read & validate the block body. - if (cls == cl_exprloc) - { - uint64_t expr_start - = cu->head->offset + read_ctx_get_offset (ctx); - if (!check_location_expression - (file, ctx, cu, expr_start, reloc, value, &where)) - return -1; - } - else - relocation_skip (reloc, - read_ctx_get_offset (ctx) + value, - &where, skip_mismatched); - - if (!read_ctx_skip (ctx, value)) - goto cant_read; - - break; } /* Relocate the value if appropriate. */ @@ -882,6 +852,7 @@ namespace << "unexpected relocation of " << pri::form (form_name) << '.' << std::endl; + form_width_t width = form->width (cu); relocate_one (&file, reloc, rel, width, &value, &where, reloc_target (form_name, it), symbolp); diff --git a/dwarflint/dwarf_version.cc b/dwarflint/dwarf_version.cc index dc8aab37c..669a5f8a6 100644 --- a/dwarflint/dwarf_version.cc +++ b/dwarflint/dwarf_version.cc @@ -33,6 +33,7 @@ #include "dwarf_4.hh" #include "dwarf_mips.hh" #include "check_debug_info.hh" +#include "pri.hh" #include "../libdw/dwarf.h" #include @@ -97,6 +98,12 @@ form::width (cu const *cu) const return static_cast (_m_width); } +std::ostream & +operator << (std::ostream &os, form const &obj) +{ + return os << pri::form (obj.name ()); +} + namespace { dw_class_set @@ -112,6 +119,12 @@ attribute::attribute (int a_name, dw_class_set const &a_classes) , _m_classes (include_indirect (a_classes)) {} +std::ostream & +operator << (std::ostream &os, attribute const &obj) +{ + return os << pri::attr (obj.name ()); +} + bool dwarf_version::form_allowed (int form) const diff --git a/dwarflint/dwarf_version.hh b/dwarflint/dwarf_version.hh index 8108b42b2..f3bee10b5 100644 --- a/dwarflint/dwarf_version.hh +++ b/dwarflint/dwarf_version.hh @@ -28,6 +28,7 @@ #define DWARFLINT_DWARF_VERSION_HH #include +#include #include "check_debug_info.ii" #include "dwarf_version.ii" @@ -131,6 +132,8 @@ public: return _m_storclass; } }; +std::ostream &operator << (std::ostream &os, form const &obj); + class attribute { @@ -154,6 +157,7 @@ public: return _m_classes; } }; +std::ostream &operator << (std::ostream &os, attribute const &obj); class dwarf_version { diff --git a/dwarflint/misc.hh b/dwarflint/misc.hh index 79b601472..fac9526d2 100644 --- a/dwarflint/misc.hh +++ b/dwarflint/misc.hh @@ -34,19 +34,19 @@ extern "C" #include "../lib/system.h" } -#define REALLOC(A, BUF) \ - do { \ - typeof ((A)) _a = (A); \ - if (_a->size == _a->alloc) \ - { \ - if (_a->alloc == 0) \ - _a->alloc = 8; \ - else \ - _a->alloc *= 2; \ - _a->BUF = (typeof (_a->BUF)) \ - xrealloc (_a->BUF, \ - sizeof (*_a->BUF) * _a->alloc); \ - } \ +#define REALLOC(A, BUF) \ + do { \ + typeof ((A)) _a = (A); \ + if (_a->size == _a->alloc) \ + { \ + if (_a->alloc == 0) \ + _a->alloc = 8; \ + else \ + _a->alloc *= 2; \ + _a->BUF = (typeof (_a->BUF)) \ + xrealloc (_a->BUF, \ + sizeof (*_a->BUF) * _a->alloc); \ + } \ } while (0) #define WIPE(OBJ) memset (&OBJ, 0, sizeof (OBJ)) @@ -58,5 +58,7 @@ bool necessary_alignment (uint64_t start, uint64_t length, bool supported_version (unsigned version, size_t num_supported, struct where *where, ...); +#define UNREACHABLE assert (!"unreachable") + #endif//DWARFLINT_MISC_HH