This is an archive of the discontinued LLVM Phabricator instance.

[MCStreamer] Move emission of attributes section into MCELFStreamer
ClosedPublic

Authored by jonpa on May 20 2021, 6:55 PM.

Details

Summary

Enable the emission of a gnu attributes section by reusing the code for emitting the ARM build attributes section.

The gnu attributes follow the exact same section format as the ARM BuildAttributes section, so this can be factored out and reused for gnu attributes generally.

The immediate motivation for this is to emit a gnu attributes section for the vector abi on SystemZ (https://reviews.llvm.org/D105067).

Diff Detail

Event Timeline

jonpa created this revision.May 20 2021, 6:55 PM
jonpa requested review of this revision.May 20 2021, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2021, 6:55 PM

Function return value / parameters: A C function with a vector parameter will be lowered by Clang differently depending on the subtarget vector support. On z13 it may be a <4 x i32>, while on zEC12 it is instead <4 x i32>*. When running instruction selection there would be then no way of telling if <4 x i32>* was a pointer argument or if it was a software ABI vector variable argument. Therefore a function attribute is emitted for each function "has-vector-arg" where as applicable. I guess the alternative might be to actually do this during instruction selection while considering the subtarget vector support presence - if using clang (and not llc), this might be enough. (E.g. if subtarget has vector support, then <4 x i32>* is really a pointer...)

This is probably not even necessary, since even a pointer to a vector would still trigger the emission of the gnu_attribute, for the same reason as:

Global Variables: the lowered types are the same regardless of subtarget, so it is simple to check them late during output.

It seems that GCC emits the vector ABI for global variables. Does this mean that they are stored differently in memory? Should e.g. variable arguments, stack arguments also be considered?

Yes, indeed. The alignment of 128-bit vector types is different between the two ABIs, which means that any use of such types, even via pointers or as struct elements, will trigger ABI differences.

Emission of the attribute: Managed to do this now as a raw text, which actually works, except with the integrated assembler... Should a new section be created for this somehow..?

For an example of how to handle such things, you might look at emitLocalEntry in the PowerPC target. You'll need a virtual function in TargetStreamer.h that can be called by the AsmPrinter.cpp code. Then there need to be two implementations of the virtual function, one for emitting assembler text, and one when directly emitting object files. Finally, you'll need to handle reading the directive in assembler code in the AsmParser.

As to how the actual implementation in object files goes, the "GNU attribute" is a flavor of the ELF "object attribute" feature. See the documentation here:
https://sourceware.org/binutils/docs-2.36/as/Object-Attributes.html
which refers to the initial specification of the build attribute section in the ARM ELF ABI document, see section "Build Attributes" here:
https://developer.arm.com/documentation/ihi0044/h/?lang=en

This isn't really Z specific, so it should probably be implemented somewhere in common code, if it isn't already ...

jonpa updated this revision to Diff 348431.EditedMay 27 2021, 7:31 PM

Patch updated - in progress.

This is probably not even necessary, since even a pointer to a vector would still trigger the emission of the gnu_attribute...
Yes, indeed. The alignment of 128-bit vector types is different between the two ABIs, which means that any use of such types, even via pointers or as struct elements, will trigger ABI differences.

Aha... I removed the function attribute emission from clang and now do all the work in SystemZAsmPrinter instead. Added a check for actually passed arguments to a vararg function.

For an example of how to handle such things, you might look at emitLocalEntry in the PowerPC target. You'll need a virtual function in TargetStreamer.h that can be called by the AsmPrinter.cpp code. Then there need to be two implementations of the virtual function, one for emitting assembler text, and one when directly emitting object files. Finally, you'll need to handle reading the directive in assembler code in the AsmParser.

I did an attempt at this except for the parser part, which seems to work. The bytes in SystemZTargetELFStreamer are simply copied from GCC output - I have no idea about the actual layout/meaing of that, except for the last two bytes...

This isn't really Z specific, so it should probably be implemented somewhere in common code, if it isn't already ...

Actually not sure what should go into common code... ARM defines ARMTargetStreamer with some attribute emission methods, but none for .gnu_attr that I can see.

Patch updated - in progress.

This is probably not even necessary, since even a pointer to a vector would still trigger the emission of the gnu_attribute...
Yes, indeed. The alignment of 128-bit vector types is different between the two ABIs, which means that any use of such types, even via pointers or as struct elements, will trigger ABI differences.

Aha... I removed the function attribute emission from clang and now do all the work in SystemZAsmPrinter instead. Added a check for actually passed arguments to a vararg function.

For an example of how to handle such things, you might look at emitLocalEntry in the PowerPC target. You'll need a virtual function in TargetStreamer.h that can be called by the AsmPrinter.cpp code. Then there need to be two implementations of the virtual function, one for emitting assembler text, and one when directly emitting object files. Finally, you'll need to handle reading the directive in assembler code in the AsmParser.

I did an attempt at this except for the parser part, which seems to work.

Missing parser support is what caused this error:

./bin/clang -o b.o -c -O3 ./b.c -march=z13 -mzvector -save-temps
b.s:27:2: error: unknown directive

which you'll only see with -save-temps (because then it reads back from the assembler file it just wrote).

The bytes in SystemZTargetELFStreamer are simply copied from GCC output - I have no idea about the actual layout/meaing of that, except for the last two bytes...

The layout is documented in the ARM ABI I refered to above. See in particular here:

The overall syntactic structure of an attributes section is:

<format-version>
[ <section-length> "vendor-name"
      [ <file-tag> <size> <attribute>*
      | <section-tag> <size> <section-number>* 0 <attribute>*
      | <symbol-tag> <size> <symbol-number>* 0 <attribute>*
      ]+
]*

Looking at your example, the bytes you output match the above format:

0x41 - format-version
0x00 0x00 0x00 0x0f - section-length
0x67 0x6e 0x75 0x00 - vendor-name ("gnu")
0x01 - file-tag
0x00 0x00 0x00 0x07 - size
Value Tag - attribute

Note that in order to properly emit this section, you cannot emit the attributes one by one, but you need to collect them somewhere and emit the whole section at the end of the file, otherwise you e.g. cannot get the section-length correct.

This isn't really Z specific, so it should probably be implemented somewhere in common code, if it isn't already ...

Actually not sure what should go into common code... ARM defines ARMTargetStreamer with some attribute emission methods, but none for .gnu_attr that I can see.

The ARM attributes go into a different section, and use a different vendor name (not "gnu"), but otherwise the format is the same as described above. So maybe the code that generates this format can be even shared between the ARM attribute section and the GNU attribute section. But in any case, the GNU attribute section in itself is common across all (ELF) platforms that use the GNU tool chain, so this part should definitely go to common code.

jonpa updated this revision to Diff 349367.Jun 2 2021, 1:59 PM

A new class MCTargetELFAttributesStreamer which contains parts taken from ARMTargetELFStreamer for emitting an attributes section. This works by using multiple inheritance so that the MCTargetStreamers that need this can derive from this new class as well (is multiple inheritance ok to use, or should it be avoided?)

SystemZAsmParser extended to handle .gnu_attribute 8, ...

This does not care about the Tag value in any other way than integer '8'. For ARM build-attributes, it e.g. is possible to do

bin/llvm-readelf --arch-specific tc_arm.s.o -

File: tc_arm.s.o
BuildAttributes {
  FormatVersion: 0x41
  Section 1 {
    SectionLength: 17
    Vendor: aeabi
    Tag: Tag_File (0x1)
    Size: 7
    FileAttributes {
      Attribute {
        Tag: 23
        Value: 3
        TagName: ABI_FP_number_model
        Description: IEEE-754
      }
    }
  }
}

Should this also be implemented for SystemZ/GNU-attributes now?

Adding more reviers with the hope to get feedback on the common code changes...

jonpa edited the summary of this revision. (Show Details)Jun 2 2021, 2:28 PM
jonpa added a reviewer: petarj.

I haven't looked at the code in detail yet, but just a couple of general comments:

  • The GNU attribute support should probably just go into the generic ELF target streamer. This concept is supported in principle on all ELF targets. (That might also avoid the need for multiple inheritance.)
  • If we support the GNU attribute generally in the ELF streamer, then we should also support it generally in the asm streamer (and then also the common asm parser, I guess)
  • ASM parser support for .gnu_attribute should support any tag&value just like GAS does.
jonpa updated this revision to Diff 351559.EditedJun 11 2021, 2:03 PM
jonpa added reviewers: craig.topper, simoncook.

The GNU attribute support should probably just go into the generic ELF target streamer. This concept is supported in principle on all ELF targets. (That might also avoid the need for multiple inheritance.)

I tried now to put it into the ELFMCStreamer class, which seems to work fine.

Is static_cast<MCELFStreamer &> safe in SystemZTargetELFStreamer (we could first check that the ObjectFormat is ELF)?

This seems possibly also interesting to RISCV which has similar code?

If we support the GNU attribute generally in the ELF streamer, then we should also support it generally in the asm streamer (and then also the common asm parser, I guess)

MCStreamer now has a virtual function emitGNUAttribute() which is implemented by both MCELFStreamer and MCAsmStreamer.

I am not quite sure how it works when ARM switches attributes sections/vendors, which seems possible but not sure it is yet used. Right now it is up to the caller to use emitGNUAttribute() / finishAttributeSection() in a sensible way...

ASM parser support for .gnu_attribute should support any tag&value just like GAS does.

MCAsmParser can now parse a .gnu_attribute in parseGNUAttribute(), which is used by SystemZAsmParser::ParseGNUAttribute() which then also checks for proper tag/value. This could perhaps be part of MCTargetAsmParser instead - although that is more "target specific"...

Not quite sure here if we should check the tag/value, or if bad .gnu_attribute directives should be reported to user. I see some errors caught:

error: Unrecognized .gnu_directive tag/value pair.
  .gnu_attribute 8, 3
                     ^
error: unknown token in expression
  .gnu_attribute bla, 2
                    ^
.gnu_attribute 9

(No error message / .gnu_attribute emitted - line ignored)

jonpa updated this revision to Diff 354629.Jun 25 2021, 4:48 PM

Move the handling of GNU attributes into MCELFStreamer. This way the target only has to call emitGNUAttribute(), and the section will later be created at the end of the file by MCELFStreamer.

Since I am not quite sure how a target might handle attributes in multiple sections (like ARM seems to do), I kept the GNU attributes separate from the other ones, so that the target can handle the Contents vector as before, while GNU attributes are collected independently of them and emitted after all else.

Nit: could createAttributesSection() take a reference of a template without the size specification? I don't think there will be 64 gnu attributes...

The SystemZ part now looks good to me, except for the point made inline. It still would be good if the rest of the patch could be looked at by someone familiar with ARM. (Maybe it would make sense to split the patch two parts?)

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
708

This is missing some cases. We need to catch any type that contains a vector type somewhere, which includes in particular aggregate types like array and structs, since the different alignment of the member may in turn trigger a different alignment of the aggregate. E.g. a struct that contains a member of vector type should trigger the attribute.

We also possibly need to catch function pointer types where the pointed-to function has a vector (or derived) type as argument or return value. (However, that may be overkill: it really only "counts" if such a type is *used* either to perform a call via function pointer, or else if the address of a local function is passed as pointer to some external user. If this is too difficult to determine, then I guess it's OK to just count all function pointer types.)

jonpa updated this revision to Diff 355063.Jun 28 2021, 4:41 PM
jonpa retitled this revision from [SystemZ] Emit .gnu_attribute for an externally visible vector abi. to [MCStreamer] Move emission of attributes section into MCElfStreamer.
jonpa edited the summary of this revision. (Show Details)
jonpa added reviewers: rengolin, t.p.northover, asl.

SystemZ specific parts moved to https://reviews.llvm.org/D105067.

This is the common-code changes that basically should be an NFC refactoring with the purpose of moving the emission of the ELF attributes section from ARM into common code, and then also reusing that for emission of a GNU section. There are still some loose ends regarding this part - see previous post.

At this point a review by someone knowledgeable with ARM is needed for this part.

jonpa retitled this revision from [MCStreamer] Move emission of attributes section into MCElfStreamer to [MCStreamer] Move emission of attributes section into MCELFStreamer.Jun 28 2021, 4:44 PM
logan added a comment.Jun 28 2021, 8:05 PM

It has been a while since I wrote the ARM attribute emitter, but anyway this commit makes sense to me and I think this is the right direction. Other than the Lint problems, I don't see other problems in this commit.

jonpa updated this revision to Diff 355302.Jun 29 2021, 11:07 AM

clang-format:ed

OK to commit?

logan accepted this revision.Jun 29 2021, 1:25 PM
This revision is now accepted and ready to land.Jun 29 2021, 1:25 PM
This revision was landed with ongoing or failed builds.Jun 30 2021, 2:01 PM
This revision was automatically updated to reflect the committed changes.