llvm dev - Dec 2019 - [RFC][DWARF] Handling errors in DWARF .debug_line parsing

Hey James,

This sounds really interesting. A few things that come to mind:

 - I'm curious what kind of errors you'd be okay with in dump mode but
not in consumer mode. From LLDB's perspective, we probably want to
extract as much information as possible from a malformed line table,
as long as we don't end up lying of course.
 - I need to take another look at the code, but don't we already have
something similar where the consumer can decide how to deal with these
kinds of issues? I'm bringing this up because I don't think we really
need different parsing modes. I think we need to let the consumer
decide what to do with the potential errors. The verifier in dwarfdump
would presumably stop after the first error (~strict mode) while the
dumper would just move on. Would the parsing modes then be a set of
"presets" with regards to how to handle these issues?
 - I'm in favor of creating these error messages lazily, especially
for LLDB where we care about responsiveness. However, that does
conflict with the behavior you want for the DWARF verifier. We
probably want a way to control this?

Cheers,
Jonas

On Mon, Dec 16, 2019 at 11:26 AM David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> (mailing lists can be busy - best to include any known-relevant parties on
the 'to' line to help highlight the discussion for them)
>
> My take on it: I'd be OK with only a strict mode with good error
messages lazily created. (so you don't get an error if you never needed to
parse/lookup a file name, for instance - don't want the strict mode to be
inefficient by aggressively parsing portions of the file you don't end up
needing) If there are cases where consumers just need to be able to read invalid
DWARF, yeah, could jump that hurdle when it comes up in a few ways I guess.
>
> On Mon, Dec 16, 2019 at 3:03 AM James Henderson via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
>>
>> Hi all,
>>
>> I'm preparing to propose some additional error checks for the DWARF
debug line parser which we have locally. However, there have been some valid
comments in the past that some errors really shouldn't prevent parsing for
certain situations. For example, a line_range of 0 would only be an issue if
anything needed that information (and would consequently divide by zero), whilst
the include directories and file names table should be terminated with null
bytes, but the parser could use the header length field to identify when the
tables have ended instead.
>>
>> After a brief discussion with Paul Robinson offline, I think it would
be good to add different parsing modes to the debug line parser. I've got
three possible categories:
>> 1) "Dump" mode - this would be the most lenient mode. It is
intended for tools like llvm-dwarfdump that merely want to print the contents to
the best of their ability, even if the table is malformed/dodgy in some other
way. It might print warnings, but these should not prevent it continuing to
parse what it can.
>> 2) "Strict" or "Verify" mode - this would be the
strictest mode, which would emit an error if it sees things like the
aforementioned bad line_range or filenames tables. Optionally, we could even
extend this to also cover things like addresses that don't make sense
(possibly with or without special handling for fixups for GC-sections-processed
output), or that could be a further mode.
>> 3) "Consumer" mode - this would be somewhere in between the
two above modes and would essentially do whatever a consumer such as LLDB wants.
It might not need to be as strict as the "Strict" mode, but some
guidance here from the consumers would be best.
>>
>> I haven't yet worked out exactly how this would work, but I think
it would probably fit into the existing error handling mechanism, either with
some extra conditionals or similar that say whether to report an error or not. I
will hopefully be putting up some proposed patches later this week that
illustrate all this.
>>
>> I'd appreciate any thoughts people have on the different modes,
whether we would need more/fewer, what they'd each cover etc.
>>
>> Regards,
>>
>> James
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Mon, Dec 16, 2019 at 2:40 PM Jonas Devlieghere <jonas at
devlieghere.com>
wrote:
> Hey James,
>
> This sounds really interesting. A few things that come to mind:
>
>  - I'm curious what kind of errors you'd be okay with in dump mode
but
> not in consumer mode. From LLDB's perspective, we probably want to
> extract as much information as possible from a malformed line table,
> as long as we don't end up lying of course.
>  - I need to take another look at the code, but don't we already have
> something similar where the consumer can decide how to deal with these
> kinds of issues? I'm bringing this up because I don't think we
really
> need different parsing modes. I think we need to let the consumer
> decide what to do with the potential errors. The verifier in dwarfdump
> would presumably stop after the first error (~strict mode) while the
> dumper would just move on. Would the parsing modes then be a set of
> "presets" with regards to how to handle these issues?
>  - I'm in favor of creating these error messages lazily, especially
> for LLDB where we care about responsiveness. However, that does
> conflict with the behavior you want for the DWARF verifier. We
> probably want a way to control this?
>
For my part - I'd imagine the verifier would be an aggressive reader of
DWARF - it'd use the same lazy/high quality error API, but the verifier
code itself would try to walk all of the parts of the DWARF to manifest any
lazy error cases, rather than needing any other codepath in the parser.

>
> Cheers,
> Jonas
>
> On Mon, Dec 16, 2019 at 11:26 AM David Blaikie via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > (mailing lists can be busy - best to include any known-relevant
parties
> on the 'to' line to help highlight the discussion for them)
> >
> > My take on it: I'd be OK with only a strict mode with good error
> messages lazily created. (so you don't get an error if you never needed
to
> parse/lookup a file name, for instance - don't want the strict mode to
be
> inefficient by aggressively parsing portions of the file you don't end
up
> needing) If there are cases where consumers just need to be able to read
> invalid DWARF, yeah, could jump that hurdle when it comes up in a few ways
> I guess.
> >
> > On Mon, Dec 16, 2019 at 3:03 AM James Henderson via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi all,
> >>
> >> I'm preparing to propose some additional error checks for the
DWARF
> debug line parser which we have locally. However, there have been some
> valid comments in the past that some errors really shouldn't prevent
> parsing for certain situations. For example, a line_range of 0 would only
> be an issue if anything needed that information (and would consequently
> divide by zero), whilst the include directories and file names table should
> be terminated with null bytes, but the parser could use the header length
> field to identify when the tables have ended instead.
> >>
> >> After a brief discussion with Paul Robinson offline, I think it
would
> be good to add different parsing modes to the debug line parser. I've
got
> three possible categories:
> >> 1) "Dump" mode - this would be the most lenient mode. It
is intended
> for tools like llvm-dwarfdump that merely want to print the contents to the
> best of their ability, even if the table is malformed/dodgy in some other
> way. It might print warnings, but these should not prevent it continuing to
> parse what it can.
> >> 2) "Strict" or "Verify" mode - this would be
the strictest mode, which
> would emit an error if it sees things like the aforementioned bad
> line_range or filenames tables. Optionally, we could even extend this to
> also cover things like addresses that don't make sense (possibly with
or
> without special handling for fixups for GC-sections-processed output), or
> that could be a further mode.
> >> 3) "Consumer" mode - this would be somewhere in between
the two above
> modes and would essentially do whatever a consumer such as LLDB wants. It
> might not need to be as strict as the "Strict" mode, but some
guidance here
> from the consumers would be best.
> >>
> >> I haven't yet worked out exactly how this would work, but I
think it
> would probably fit into the existing error handling mechanism, either with
> some extra conditionals or similar that say whether to report an error or
> not. I will hopefully be putting up some proposed patches later this week
> that illustrate all this.
> >>
> >> I'd appreciate any thoughts people have on the different
modes, whether
> we would need more/fewer, what they'd each cover etc.
> >>
> >> Regards,
> >>
> >> James
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191216/1cc484d0/attachment.html>

On 16/12/2019 23:47, David Blaikie via llvm-dev wrote:
> 
> 
> On Mon, Dec 16, 2019 at 2:40 PM Jonas Devlieghere <jonas at
devlieghere.com
> <mailto:jonas at devlieghere.com>> wrote:
> 
>     Hey James,
> 
>     This sounds really interesting. A few things that come to mind:
> 
>      - I'm curious what kind of errors you'd be okay with in dump
mode but
>     not in consumer mode. From LLDB's perspective, we probably want to
>     extract as much information as possible from a malformed line table,
>     as long as we don't end up lying of course.
>      - I need to take another look at the code, but don't we already
have
>     something similar where the consumer can decide how to deal with these
>     kinds of issues? I'm bringing this up because I don't think we
really
>     need different parsing modes. I think we need to let the consumer
>     decide what to do with the potential errors. The verifier in dwarfdump
>     would presumably stop after the first error (~strict mode) while the
>     dumper would just move on. Would the parsing modes then be a set of
>     "presets" with regards to how to handle these issues?
>      - I'm in favor of creating these error messages lazily, especially
>     for LLDB where we care about responsiveness. However, that does
>     conflict with the behavior you want for the DWARF verifier. We
>     probably want a way to control this?
> 
> 
> For my part - I'd imagine the verifier would be an aggressive reader of
> DWARF - it'd use the same lazy/high quality error API, but the verifier
> code itself would try to walk all of the parts of the DWARF to manifest
> any lazy error cases, rather than needing any other codepath in the parser.
That would be my ideal setup as well (disclaimer: I work on lldb) --
have the parser provide sufficient information so that the verification
can be done from the "outside".

That is, if the goal is to have stronger verification of generated line
tables -- it's not fully clear (to me) whether that's your main
motivation for adding these checks.

regards,
pavel