[PATCH] D84386: [DWARFYAML] Add support for emitting custom operands for range list entry.

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D84386#2168872 <https://reviews.llvm.org/D84386#2168872>, @Higuoxing wrote:

> In D84386#2168871 <https://reviews.llvm.org/D84386#2168871>, @jhenderson wrote:
>
> > In D84386#2168864 <https://reviews.llvm.org/D84386#2168864>, @labath wrote:
> >
> > > In D84386#2168813 <https://reviews.llvm.org/D84386#2168813>, @jhenderson wrote:
> > >
> > > > In general terms, I think this is a good solution. Let's see what @grimar and @labath think.
> > >
> > >
> > > Pretty much what I said already. This might be a nice way to test parsing of unknown/invalid DW_LLE constants, but there's only so many of those kinds of tests that we need, so it seems unnecessary.
> > >
> > > Furthermore, once we're down in the invalid input land, it becomes very important to know how the bytes are laid out exactly in order to understand what the parser will do/has done. This becomes a (small) obstacle in that because I have to reason about the actual byte sequence that will be produced by this yaml -- I would prefer if I just had the bytes directly.
> > >
> > > But if you do want to have this, I'm not going to stop you...
> >
> >
> > @Higuoxing, could you put up a patch that uses the hex approach @labath is suggesting instead of this approach, please? We can then better compare the complexity etc of the two approaches.
>
>
> Yeah, no problem!


The biggest advantage I see in that approach is that it would enable obj2yaml to describe (chunks of) invalid debug_rnglists sections. So the way I would do this is to approach this from the other end -- look at how obj2yaml does its parsing. The point where it would need to "give up" because of a parsing error is a good place to insert a `Contents` attribute with a hex string. Here, one good such place seems to be the level which describes a single range list -- if one range list doesn't parse, we just insert a binary blob and then try again with the next list.



================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:531
+
+  switch ((uint8_t)Entry.Operator) {
   case dwarf::DW_RLE_end_of_list:
----------------
Higuoxing wrote:
> grimar wrote:
> > Do you need a cast here?
> I'm not sure about it here. Seems that some build bots are not happy if we do not add a cast here since we have covered all enumeration values in this switch.
This looks like the "default label in a fully covered switch" warning, except that technically the switch is *not* fully covered without the default case. C++ is weird...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84386/new/

https://reviews.llvm.org/D84386