This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Rewrite ARMAttributeParser
ClosedPublic

Authored by MaskRay on Feb 22 2020, 5:15 PM.

Details

Summary
  • Delete boilerplate
  • Change functions to return Error
  • Test parsing errors
  • Update callers of ARMAttributeParser::parse() to check the Error return value.

Since this patch touches nearly everything in the file, I apply
http://llvm.org/docs/Proposals/VariableNames.html and change variable
names to lower case.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 22 2020, 5:15 PM
MaskRay added a comment.EditedFeb 22 2020, 5:23 PM

The format of .ARM.attributes can be found at https://developer.arm.com/docs/ihi0044/h/elf-for-the-arm-architecture-abi-2019q1-documentation

In BFD, some other architectures define elf_backend_obj_attrs_vendor: bfd/elf32-msp430.c bfd/elf32-tic6x.c bfd/elf32-arc.c bfd/elfnn-riscv.c.

MaskRay updated this revision to Diff 246110.Feb 23 2020, 8:59 AM
MaskRay added a reviewer: samparker.

Fix two tests

This is a large change and converting to lower case is making really hard to discern what the functional changes are.

This is a large change and converting to lower case is making really hard to discern what the functional changes are.

Pasted my reply in D74023.

For existing code, there is objection that clean-ups can clutter up the history. There is a supportive group of people putting forward a migration plan. For *AttributeParser, I think it is a middle ground: the rewrite (refactoring) will touch nearly every line (we will also face a large function move ARMAttributeParser.cpp -> ELFAttributeParser.cpp). I am in the camp of "I don't want to unnecessarily break the style of surrounding code, but if it is new code, I want to just use the ideal style." So I went forward and fixed the variable Naming in D75015.

grimar added a comment.EditedFeb 25 2020, 1:56 AM

This is a large change and converting to lower case is making really hard to discern what the functional changes are.

+1. It does not feel right that mixing of code style and functional changes happens.

llvm/lib/Support/ARMAttributeParser.cpp
500

Seems it can be:

if (sw) {
  DictScope scope(*sw, scopeName);
  if (!indicies.empty())
    sw->printList(indexName, indicies);
} 

if (Error e = parseAttributeList(size))
  return e;
llvm/tools/llvm-readobj/ELFDumper.cpp
2693–2695

TODO:

2696

Excessive curly braces around a single line.

MaskRay marked 2 inline comments as done.Feb 25 2020, 10:14 PM
MaskRay added inline comments.
llvm/lib/Support/ARMAttributeParser.cpp
500

It can't.

DictScope changes the indentation level. parseAttributeList must be called with DictScope in the scope.

MaskRay updated this revision to Diff 246990.Feb 27 2020, 9:37 AM
MaskRay marked 3 inline comments as done.

TODO -> TODO:
Delete an excess pair of braces

MaskRay updated this revision to Diff 248538.Mar 5 2020, 10:22 AM

invalid CPU_arch value, expected to be inside [0,22)
->
unknown CPU_arch value: 22

compnerd accepted this revision.Mar 5 2020, 10:27 AM
compnerd added inline comments.
lld/test/ELF/arm-tag-vfp-args-illegal.s
5

I think that the original error message was better here

llvm/include/llvm/Support/ARMAttributeParser.h
42

A newline before this would be nice to delimit the general handler vs the known attribute list.

78

It might be nice to use a XMACRO here. But thats not critical/important.

82

Would be nice to group this with the other general handlers.

87

static_cast<void> please.

llvm/lib/Support/ARMAttributeParser.cpp
67

Please put a trailing comma after the nodefaults handler and re-clang-format.

401

I think that the original code was clearer here. I think at the very least, we should have a comment explaining this offset.

416

Bleh, could you please check if we have a constant for this yet? IIRC, we didn't have one when I wrote this, but really this is the limit of the namespace and should really have a constant.

446

Again, I think that the original code was more readable here. Please use sizeof(length) or add a comment explaining the 4.

This revision is now accepted and ready to land.Mar 5 2020, 10:27 AM
MaskRay updated this revision to Diff 248547.Mar 5 2020, 10:44 AM
MaskRay marked 10 inline comments as done.

Address comments

MaskRay updated this revision to Diff 248550.Mar 5 2020, 10:54 AM

Fix an lld test

MaskRay added inline comments.Mar 5 2020, 10:56 AM
llvm/lib/Support/ARMAttributeParser.cpp
401

Moved - 5 to the caller.

416

Will do afterwards.

446

Moved - 4 to the caller. I think it will be more readable.

This revision was automatically updated to reflect the committed changes.
lld/test/ELF/arm-tag-vfp-args-illegal.s