This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

dwarflint code review


Overall it looks fine, a great start.

There are some cosmetic nits: { should always be on its own line;
comment lines don't use leading "*"; we don't use StudlyCaps in new
internal code, it's only for public type names; no need for __extension__
in our code, only for public headers.

Use xmalloc, xcalloc, xrealloc, rather than checking for failure.

Use assert rather than conditional calls to abort.

Use macros rather than 'static const size_t' for constants.

In read_ctx_read_var and similar cases, I'd use a switch instead of 'if's.

Use bool for storing 'has_children'.  Just check its validity at the
decoding site.

There is no structural validity check for tag values, except that they are
not zero and not greater than INT_MAX.  Likewise for attr names.  These are
not "structural" elements--we can navigate the tree just fine without
knowing them, and they will not confuse libdw or its interfaces.

Don't say "non-NULL abbrev" and "NULL abbrev" in messages.  "NULL" in caps
is a special term for pointers in C and C++.  If "null" makes sense in
context, use it as a normal word.  Here, I think you mean "zero" or "empty"
or something else like that.

Each warning/error message should give the section name and offset where
the suspicious data is found.  (Many do, but some don't.)

The DW_FORM_ref_addr encoding depends on the DWARF version number field in
the CU header.  For version==2, it's address_size.  For 3, it's offset_size.

There is no validity check you need to do at the structural level for
DW_FORM_addr.  We'll consider semantic attribute value checks in the higher
layer checks later on.  The checks we have so far are only to handle the
structural level of stuff that could confuse libdw or that libdw would fail
to notice.

Likewise, string/strp don't need a structural check beyond being properly
terminated and not running off the end of the section (and the .debug_str
coverage checks).  The contents of the string are for a higher-level value
check.

DW_FORM_flag could use a validity check for a value not 0 or 1.
This is not "structural" per se, but is information that libdw
loses (in dwarf_formflag), so we can't check it at higher level.


Thanks,
Roland

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]