Page MenuHomePhabricator

[DebugInfo] Add support for DWARF64 into DWARFDebugAddr.
ClosedPublic

Authored by ikudrin on Thu, Feb 6, 10:53 PM.

Diff Detail

Event Timeline

ikudrin created this revision.Thu, Feb 6, 10:53 PM
aprantl accepted this revision.Fri, Feb 7, 8:47 AM
aprantl added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
68

DiagnosticLength?

161

unsigned?

This revision is now accepted and ready to land.Fri, Feb 7, 8:47 AM
dblaikie added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
55–75

@labath another instance of this 32/64 length parsing/error dance and subsequent "isValid" checking following it that'd be great to tidy up with a common utility for parsing the length and returning a length-constrained DWARFDataExtractor for further use.

ikudrin marked 2 inline comments as done.Sat, Feb 8, 2:24 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
68

Thanks! I'll change the name, as well as for the other similar instances.

161

Could you explain why unsinged is better than int here?

Note that according to http://www.cplusplus.com/reference/cstdio/printf/,

*: The width is not specified in the format string, but as an additional integer value argument preceding the argument that has to be formatted.

Not "an unsigned integer value".

ikudrin updated this revision to Diff 243370.Sat, Feb 8, 7:45 AM
  • Update to reflect changes in parent revisions.
  • TmpLength -> DiagnosticLength.
  • Added a test for insufficient space for an extended length field.
jhenderson added inline comments.Mon, Feb 10, 1:07 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
57

IIRC, the DataExtractor class provides an optional second argument to its get... methods that can be used to specify an Error to pass any problems back in, including reading past the end. Would it make some sense to use that instead of the isValid... checks?

72–74

Maybe errc::not_supported?

161

I find the following link tends to be more precise and accurate in its wording:

https://en.cppreference.com/w/cpp/io/c/fprintf

(optional) integer value or * that specifies minimum field width. The result is padded with space characters (by default), if required, on the left when right-justified, or on the right if left-justified. In the case when * is used, the width is specified by an additional argument of type int. If the value of the argument is negative, it results with the - flag specified and positive field width. (Note: This is the minimum width: The value is never truncated.)

I don't have a copy of the standard to see whether the type is explicitly mentioned, but that quote definitively says int.

aprantl added inline comments.Mon, Feb 10, 4:45 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
161

Cool, I wasn't aware of that.

ikudrin marked an inline comment as done.Tue, Feb 11, 1:45 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
161

Oh, that reference is great. They even have a sample with a negative value for field width.

jhenderson added inline comments.Tue, Feb 11, 1:53 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
161

Huh, I didn't notice that, nor did I realise that left-justification was even possible!

ikudrin marked an inline comment as done.Tue, Feb 11, 5:42 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
57

Can't say I like that because the resulting diagnostic messages might not be that informative as with the current approach.

ikudrin updated this revision to Diff 243853.Tue, Feb 11, 6:46 AM
  • Updated to reflect changes in parent revisions.
  • Changed errc::invalid_argument to errc::not_supported for the message for a reserved unit length value.
This revision was automatically updated to reflect the committed changes.