Page MenuHomePhabricator

[DWARFYAML] Add support for emitting custom operands for range list entry.
AbandonedPublic

Authored by Higuoxing on Jul 22 2020, 11:14 PM.

Details

Summary

This patch adds support for emitting custom operands for range list.

Diff Detail

Event Timeline

Higuoxing created this revision.Jul 22 2020, 11:14 PM

Remove an unused header.

Higuoxing updated this revision to Diff 280044.Jul 23 2020, 1:02 AM

Suppress clang-tidy warning.

In general terms, I think this is a good solution. Let's see what @grimar and @labath think.

llvm/include/llvm/ObjectYAML/DWARFYAML.h
188

enum class?

439–445

It might be nice to call these U8, U16 etc, to make them nice and concise.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
655–661

Maybe we should just treat this case as having no operands automatically?

676

I think you can just delete "their" here.

Looks fine to me either.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
478

Does it make sense to print the number of types and values in the message?
e.g.

"the number of operands (5) should match the number of their types (4)"

538

Do you need a cast here?

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
650

What about adding a test with mismatching types/values.
e.g

Types:    [ Unsigned8 ]
Values:   [ 0x1234567890abcdef ]

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...

llvm/include/llvm/ObjectYAML/DWARFYAML.h
439–445

+1

Higuoxing marked an inline comment as done.Jul 23 2020, 2:30 AM
Higuoxing added inline comments.
llvm/lib/ObjectYAML/DWARFEmitter.cpp
538

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.

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.

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!

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.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
538

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...

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.

I've implemented one in D84618 :-) Any thoughts?

I think the other version is better, so you can probably abandon this change.

Higuoxing abandoned this revision.Jul 28 2020, 7:05 PM

This patch is abandoned in favor of D84618.