Page MenuHomePhabricator

[RISCV] ELF attribute section for RISC-V
Needs ReviewPublic

Authored by HsiangKai on Wed, Feb 5, 12:43 AM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
HsiangKai updated this revision to Diff 242557.Wed, Feb 5, 4:33 AM

Fix some typos.

khchen added a subscriber: khchen.Thu, Feb 6, 7:06 AM

I believe more tools like llvm-readobj also would need to parse the attribute structure, and even lld.

There is a Attribute parser in llvm/Support/ but it looks like it supports only ARM at this point.

Are you planning to add support to parse RISCV inside of LLVM Support (or) enhance the attribute parser to be more generic ?

Refactor ELF attribute parser to support ARM and RISC-V target-specific attribute section.

Address fixups.

(I don't know anything about RISC-V - my comments are purely from an llvm-readobj developer's point of view).

llvm/test/tools/llvm-readobj/ELF/riscv-attribute.s
1 ↗(On Diff #244086)

This is going to need a REQUIRES, right?

Also, we prefer to have a top-level comment in new llvm-readobj tests that explains the point of the test.

I also wonder whether it would be less circular in the testing if you added support in yaml2obj to generate such a section, and then use that to write the llvm-readobj test. I'm not sure either way though.

HsiangKai updated this revision to Diff 244161.Wed, Feb 12, 6:31 AM

Create RISCV directory under test/tools/llvm-readobj/ELF for RISCV test cases.

Latest changes look fine from the llvm-readobj point of view.

HsiangKai updated this revision to Diff 244171.Wed, Feb 12, 7:09 AM
  1. Add description of test purpose in the front of test cases.
  2. Use yaml2obj to generate object files and use llvm-readobj to decode it.
jhenderson added inline comments.Wed, Feb 12, 7:26 AM
llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test
7

Add a space between the '#' and the "CHECK" here and below.

37

When I said "add support to yaml2obj", I meant adding something to yaml2obj that makes these things a bit more section- specific, a bit like the relocation sections. That way, the YAML blob is not as opaque as this. I'd prefer an assembly test over something opaque.

HsiangKai updated this revision to Diff 244179.Wed, Feb 12, 7:37 AM

Sorry, I didn't see your newest comments before I updated the revision. About adding support to yaml2obj, I will think about it. Could we prepare another patch for yaml2obj modification?
I found that I didn't take care 64-bits formatting. I will focus on taking care of 64-bits formatting first.

Sorry, I didn't see your newest comments before I updated the revision. About adding support to yaml2obj, I will think about it. Could we prepare another patch for yaml2obj modification?
I found that I didn't take care 64-bits formatting. I will focus on taking care of 64-bits formatting first.

A separate patch for yaml2obj would be fine from my point of view. Also, regarding the 64-bit formatting, consider putting 32-bit and 64-bit formatting in the same test and using the new yaml2obj option "-D" to share the code, i.e. something like:

# RUN: yaml2obj %s -D BITS=32 -o %t.32
# RUN: yaml2obj %s -D BITS=64 -o %t.64

--- !ELF
FileHeader:
  Class:   ELFCLASS[[BITS]]
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_RISCV
etc
HsiangKai updated this revision to Diff 244326.Wed, Feb 12, 8:11 PM

Update test cases.

HsiangKai updated this revision to Diff 244356.Thu, Feb 13, 1:19 AM

Apply attribute values to RISC-V attributes and add test cases.

evandro added inline comments.Thu, Feb 13, 8:38 AM
llvm/lib/Object/ELFObjectFile.cpp
318
switch (Arch[0]) {
case 'i':
case 'e':
...
case 'c':
  Features.AddFeature(Arch[0]);
}
HsiangKai updated this revision to Diff 244560.Thu, Feb 13, 6:49 PM

Address Evandro's comments.

jhenderson added inline comments.Fri, Feb 14, 2:40 AM
llvm/include/llvm/Support/ELFAttributeParser.h
2

This is missing the language specifier.

llvm/include/llvm/Support/RISCVAttributeParser.h
2

This is missing the language specifier.

llvm/lib/Support/ELFAttributeParser.cpp
45

Use /*TagPrefix=*/ - clang-format recognises this pattern and removes the space after it.

61

Ditto.

127

Use C++ style comment.

HsiangKai updated this revision to Diff 244825.Sat, Feb 15, 8:28 AM

Address @jhenderson's comments.

MaskRay added inline comments.Mon, Feb 17, 3:47 PM
llvm/include/llvm/Support/ELFAttributeParser.h
32

Use camelCase function names.

You can even start to use variableName for variables. Because this patch adds many new files. They don't need to get stuck with existing VariableName rule which is considered broken https://lists.llvm.org/pipermail/llvm-dev/2019-February/129907.html

llvm/include/llvm/Support/ELFBuildAttributes.h
22 ↗(On Diff #244825)

include llvm/ADT/ArrayRef.h

jhenderson added inline comments.Tue, Feb 18, 2:30 AM
llvm/lib/Support/ELFBuildAttrs.cpp
1 ↗(On Diff #244825)

You don't need the language specifier for .cpp files. It is needed by some IDEs to detect the language of headers, but can be derived from the file extension in source file cases. See https://llvm.org/docs/CodingStandards.html#file-headers for details.

pzheng added inline comments.Tue, Feb 18, 4:00 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
102

should be "Streamer.emitIntValue"

same for all other "Streamer.emitXXX" references

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
102–103

void emitDirectiveOptionPush() override;

same for all the emitDirectiveOption functions below

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
64

should be emitStartOfAsmFile ?

65

same here, emitEndOfAsmFile

164

emitStartOfAsmFile ?

171

emitEndOfAsmFile ?

HsiangKai marked 2 inline comments as done.Tue, Feb 18, 7:33 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
102

The interface is defined by MCStreamer.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
64

The interface is defined in AsmPrinter.

MaskRay added inline comments.Tue, Feb 18, 9:06 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
102

These MCStreamer::Emit* functions have been de-capitalized.

HsiangKai updated this revision to Diff 245321.Tue, Feb 18, 9:32 PM

Address comments and rebase on master branch.

pzheng added inline comments.Wed, Feb 19, 4:42 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
3492

should also handle SHT_RISCV_ATTRIBUTES here

apazos added inline comments.Wed, Feb 19, 5:25 PM
llvm/test/MC/RISCV/attribute-with-insts.s
6

Kai, do you plan to add some llvm-readelf tests?

HsiangKai marked an inline comment as done.Thu, Feb 20, 8:52 AM
HsiangKai added inline comments.
llvm/test/MC/RISCV/attribute-with-insts.s
6

I found that llvm-readelf is a symbolic link to llvm-readobj. So, there is no need to add test cases for llvm-readelf.

HsiangKai updated this revision to Diff 245670.Thu, Feb 20, 9:02 AM
MaskRay added inline comments.Thu, Feb 20, 11:46 AM
llvm/include/llvm/Support/ELFAttributeParser.h
52

Add a public virtual destructor.

llvm/include/llvm/Support/ELFBuildAttributes.h
22 ↗(On Diff #245670)

omit struct

26 ↗(On Diff #245670)

These enum constants are defined in binutils-gdb/bfd/elf-bfd.h.

I am unsure this file should be named ELFBuildAttributes.h

ELFAttributes.h is probably fine.

llvm/include/llvm/Support/RISCVAttributes.h
26

Add const

llvm/lib/Support/ELFBuildAttrs.cpp
14 ↗(On Diff #245670)

Delete namespace llvm {.

llvm/lib/Support/RISCVAttributeParser.cpp
16

Delete namespace llvm {

llvm/lib/Support/RISCVAttributes.cpp
15

Add const

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1719

Use one variable for IsStringValue and IsIntegerValue.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
80

Inconsistent.

clang-format uses this form: /*OverwriteExisting=*/true

122

range-based for loop

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
21

Is = 0 significant?

25

omit Type

71

End with a period.

74

Use initializer lists. Contents.push_back({.......})

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
30

Arch = "rv32"

33

ditto

93

"\"\n"

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
32

Can we avoid the default value?

llvm/test/MC/RISCV/attribute-with-insts.s
6

llvm-readobj and llvm-readelf have different output styles, so sometimes there may need two sets of tests. See test/tools/llvm-readobj/ GNU: LLVM:

I haven't checked this patch, though.

7

Indent continuation lines by 2.

llvm/test/MC/RISCV/attribute.s
2

## for comments. They make comment lines stand out from RUN and CHECK lines.

4

No need for a continuation line

llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test
70

Second to jhenderson's suggestion. This opaque content is not necessarily better than an assembly test.

HsiangKai updated this revision to Diff 245807.Fri, Feb 21, 2:04 AM
HsiangKai marked an inline comment as done.

Address @MaskRay's comments.

HsiangKai added inline comments.Fri, Feb 21, 2:08 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
25

Type is a data member of AttributeItem.

HsiangKai updated this revision to Diff 245808.Fri, Feb 21, 2:08 AM
MaskRay added inline comments.Fri, Feb 21, 10:45 AM
llvm/include/llvm/Support/ELFAttributes.h
34

period

llvm/include/llvm/Support/RISCVAttributeParser.h
1

Nit: ===-- RISC-V

llvm/lib/Object/ELFObjectFile.cpp
297

consume_front and delete drop_front(4)

308

The defualt branch is still reachable, thus it is incorrect to use llvm_unreachable.
You may want to return an Error.

llvm/lib/Support/ARMBuildAttrs.cpp
11

Use using namespace llvm;

See https://reviews.llvm.org/D74515

llvm/lib/Support/ELFAttributeParser.cpp
137

Delete braces

142

//

MaskRay added inline comments.Fri, Feb 21, 11:01 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
87

/* -> /*

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
25

It is not common to use enum { ... } Type in C++. Type is not referenced anyway.

56

"" is sufficient