This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML] - Redefine LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::yaml::Hex*) as LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR.
ClosedPublic

Authored by grimar on Oct 29 2019, 5:04 AM.

Details

Summary

I am using it in https://reviews.llvm.org/D69399.

This change changes how obj2yaml dumps arrays of llvm::yaml::Hex8/llvm::yaml::Hex16/llvm::yaml::Hex32
from:

PayloadBytes:
- 0x01
- 0x02
...

To

PayloadBytes:    [ 0x01, 0x02, ... ]

The latter way is shorter and looks better for arrays.

Diff Detail

Event Timeline

grimar created this revision.Oct 29 2019, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 5:04 AM

Looks fine to me, aside from one concern.

llvm/test/ObjectYAML/MachO/bogus_load_command.yaml
40

Not knowing much about Mach-O, I'm slightly concerned that these extra four zeros were never intended and might be highlighting a bug. @alexshap/@seiya etc might have a better idea?

grimar marked an inline comment as done.Oct 29 2019, 5:26 AM
grimar added inline comments.
llvm/test/ObjectYAML/MachO/bogus_load_command.yaml
40

Here is the output without this patch applied:

  - cmd:             0xDEADBEEF
    cmdsize:         24
    PayloadBytes:
      - 0x01
      - 0x02
      - 0x03
      - 0x04
      - 0x05
      - 0x06
      - 0x07
      - 0x08
      - 0x09
      - 0x0A
      - 0x0B
      - 0x0C
      - 0x00
      - 0x00
      - 0x00
      - 0x00
...

So these bytes were there before my change, but have never been tested/shown in the test case.

jhenderson added inline comments.Oct 29 2019, 5:35 AM
llvm/test/ObjectYAML/MachO/bogus_load_command.yaml
40

Yes, that's what I assumed. My concern is whether they should be there at all, as they weren't being tested before. It's possible the writer of this test didn't realise they were being emitted and didn't want them to be in this case.

MaskRay resigned from this revision.Oct 29 2019, 9:30 AM

It seems the problem in ObjectYAML/MachO/bogus_load_command.yaml was introduced before this patch, and Mach-O developers have been notified. LG

BTW, have you tested lldb? It recently gets some YAML tests.

llvm/test/ObjectYAML/MachO/DWARF2-AddrSize8-FormValues.yaml
441–442

Just a thought: there are 7 elements on one line. 8 may look slightly better if we can make the column limit softer..

The default WrapColumn used in llvm::yaml::Output::Output is 70. YAMLTraits tries to wrap the line when the column number exceeds 70.

bool Output::preflightFlowElement(unsigned, void *&) {
  if (NeedFlowSequenceComma)
    output(", ");
  if (WrapColumn && Column > WrapColumn) {
    output("\n");
    for (int i = 0; i < ColumnAtFlowStart; ++i)
      output(" ");
    Column = ColumnAtFlowStart;
    output("  ");
  }
  return true;
}
MaskRay accepted this revision.Oct 29 2019, 9:30 AM
This revision is now accepted and ready to land.Oct 29 2019, 9:30 AM
grimar marked an inline comment as done.EditedOct 30 2019, 3:43 AM

BTW, have you tested lldb? It recently gets some YAML tests.

<there was something here saying it is not needed, but see below>

Upd: I am taking the words above back. I've found LLDB is also affected. Thanks!

llvm/test/ObjectYAML/MachO/DWARF2-AddrSize8-FormValues.yaml
441–442

Yeah. And we probably might remove the excessive identation too. I.e. make obj2yaml produce the output which is often requested during reviews anyways. This would save some space for softening of this limit here and in other possible places.

grimar planned changes to this revision.Oct 30 2019, 4:07 AM

Returning to this, LLDB uses YAMLs in the old form for inputs. But this patch only affects output. We accept both the old and the new way for describing arrays.
(MachO tests in this diff do the same, they read the old style and check the YAML produced which is in a new style)
So, LDLB tests are fine.

But there is another problem: this patch never tests we can read arrays in the new style.
I am going to add few tests to Support/YAMLIOTest.cpp and update this diff.

grimar updated this revision to Diff 227064.EditedOct 30 2019, 4:44 AM
  • Add test cases to SupportTests to check that we can read arrays of Hex* described in a new output style.
  • Make Hex16 to be LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR too.
This revision is now accepted and ready to land.Oct 30 2019, 4:44 AM
grimar requested review of this revision.Oct 30 2019, 4:44 AM
jhenderson accepted this revision.Oct 30 2019, 6:23 AM

Looks good to me. I still think the Mach-O test point needs addressing, but that doesn't necessarily need to block this commit.

This revision is now accepted and ready to land.Oct 30 2019, 6:23 AM
This revision was automatically updated to reflect the committed changes.