[yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[Esme]

Summary

This patch implements the mapping of the Yaml information to XCOFF object to enable the operation of yaml2obj.
Currently only 32-bit mode is supported.

[jhenderson]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

58

It might be a good idea to break this function up into smaller pieces, perhaps one function per loop, so you'd have a function that assigns the section offsets and addresses, another function for the relocations, another for the symbols etc.

82

Put a blank line before this line, to help break up this function.

Same goes below: if you put a comment, followed immediately by an if statement or loop relating to that comment, add a blank line.

Also, I think it's more common grammatically in code comments to right "Assign" rather than "Assigns" (i.e. use the imperative form). Same for "Calculates" (use "Calculate") below.

94

Is "FileOffsetToData" the actual XCOFF field name, or is it something else? If something else, I recommend using the real field name.

96–97

And check whether the comment can be reflowed to fit within 80 characters width.

103

1. Can there be more than one each of .text/.data/.bss?
2. If so, are they all named the same?
3. Are the names actually ".text", ".data" etc?

110

If I'm reading this code correctly, if a user has put their sections in an order like:

.text
.somethingelse
.data

all sections are size 4, and the address of .text is 16, the address of .data is going to be 24, not 20. Is that supposed to be the case?

116

Reading this code, it looks like you can't have in XCOFF relocations that aren't attached to a section. Is that correct?

148

You probably need to make the Header fields optional themselves, so that this code can distinguish between the case where the user has explicitly specified the value as 0 and the case where it is unspecified.

The same likely goes for other structs like the Section.

Perhaps also worth pulling 0x01DF into a named constant somewhere.

149

Where the field name is self-explanatory, you don't need to have a comment. Same goes below.

175–177

Calculate the requested address value upfront, then use that value in both places, rather than repeat the logic to calculate it.

208

Same in similar messages below.

222

Probably best to make all these PaddingSize fields int64_t to prepare for 64-bit support.

225

272

At this point, it may be better to just use write_zeroes to fill the whole auxiliary entry. Alternatively, emit an error if NumberOfAuxEntries is non-zero.

290

This sounds like it prevents a user from writing a 32-bit XCOFF file, but with a messed up magic field. Perhaps you should put an optional explicit format field in the YAML (defaulting to 32-bit XCOFF), and use that to determine the file format to use.

295

It seems like if you failed to assign addresses or indices, it may be dangerous to continue? This should probably bail out. Same probably goes for the other cases where an error can occur too.

llvm/lib/ObjectYAML/XCOFFYAML.cpp

113

In ELF, the r_offset field of a relocation specifies where the relocation will patch, and is relative to either the section start or file start, depending on the context. Often in our test cases, it doesn't really matter where the relocation is patching, only that there is a relocation, hence the offset field is optional there. It seems likely that this will be the case for XCOFF too?

114

Can XCOFF relocations have no symbol? If so, I think a 0 index value would be a reasonable default.

116

Is there a reasonable default for the relocation type? In ELF, there isn't really, so I think it's a required field.

[Esme]

Addressed jhenderson's comments.

[Esme]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

94

Yes, it's also defined in XCOFF.h.
The name "FileOffsetToData" has been used in all XCOFF related implementations，which corresponds to "s_scnptr" in the documentation.

103

1. Yes, multiple .text/.data/.bss sections are allowed.
2. Applications use the Flags field instead of the Name field to determine a section type. Two sections of the same type may have different names.
3. As introduced in Table 4.

110

Moving this calculation ahead of the calculation of FileOffsetToData gives the correct result.

116

Yes, the relocations are always attached to sections in XCOFF.

llvm/lib/ObjectYAML/XCOFFYAML.cpp

113

I agree that it's unfriendly to require the virtual address of the relocation from users.
While this item in XCOFF specifies the virtual address of the value that requires modification by the binder. And the offset to the data in the section will be calculated as follows:
offset_in_section = Relocation_Address - Section_Address
We can calculate the Section_Address from yaml contents, but hard to determine the Relocation_Address.
So this is hardly optional?

120

It's reasonable to make the name to be optimal and the flags to be required, since the flags are the unique identifier of the section type.

[Esme]

Only the .text, .data, .tdata, and STYP_DWARF sections have relocations.

[jhenderson]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

56–57

Why not just use Obj.Header and Obj.Sections directly in the referencing code? Why do you need these member variables separately to Obj?

63–64

Is this a fundamental restriction of the XCOFF file format, or is it just what should happen? Is it possible to create (potentially by hand) a section with relocations if it isn't one of these types?

In general, to allow for testing of bad input paths etc, you want to allow as much flexibility in yaml2obj as possible. As such, it isn't yaml2obj's place to restrict what can be done, as long as it can physically represent what was requested.

77

MaxRawDataSize is an internal variable, right, not an XCOFF spec defined value? If MaxRawDataSize doesn't directly appear in the spec, don't mention it to the user in an error message. Instead, use general terms like "the maximum size permitted for XXX" (where XXX is the thing that is restricted, e.g. the object size).

103

It looks like from the spec that the names are merely conventions, so technically the names could be other thigns. It's probably fine to infer that if a section is text, it is called .text, unless otherwise specified by the YAML (and vice versa), but this bit applies to the section type, not the name, so we should refer to them by type rather than name (i.e. just "text", "data", "bss", not ".text" etc).

120

Is it definitely the address here that should be being aligned, or the offset? The two are different concepts, and the align function appears to align the offset, not the address.

136

191–192

Here, it's probably worth a comment saying which is the virtual address and which the physical address, like the inline edit, for example (which may be the wrong way around).

llvm/lib/ObjectYAML/XCOFFYAML.cpp

113

Could 0 be a reasonable default? If not, it's fine. I'm just wondering if it actually matters in all cases what the address is.

120

Sounds good to me.

[Esme]

Addressed James's comments.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

56–57

Obj can't be changed in assignAddressesAndIndices(), because it will be used during writing. The value specified in the YAML has a higher priority to be written than the calculated valued derived from the contents. And only Header and Sections have these number/offset/address fields which need to be calculated, therefore I use InitSections and InitFileHdr to keep these calculated values.

120

The offset of data for each section is what I want to align. The comments may be confusing here.

llvm/lib/ObjectYAML/XCOFFYAML.cpp

113

As I see, 0 is not a reasonable default.

[Esme]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

63–64

It is just what should happen.

Thanks for the advice, it makes sense to me to removing the restriction to allow more flexibility.

[jhenderson]

Some more minor comments. I'll take a look at the tests next time.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

68

Sorry, I might have confused you with my previous comment. MaxRawDataSize is being compared to CurrentOffset, so it's not the size of the relocations/sections that is being constrained, if I read it correctly. Perhaps you could change the message to something like "maximum object size of XXX exceeded when writing relocation data". What do you think?

91

How about this?

137–138

Same as relocation comment above.

[jhenderson]

FYI, I will be off for just over two weeks from Thursday. Please don't feel the need to wait on further comments from me, if there are other yaml2obj developers that are happy with this change. Same goes for your other changes for llvm-objcopy and obj2yaml.

[Higuoxing]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

14

Do we need this header file? I guess what we need is DenseMap ?

128

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml

25–30

It looks that the Value, Type, StorageClass and NumberOfAuxEntries are optional too. Could you please add some test cases for them as well?

[Esme]

Thank you for your review! @jhenderson and @Higuoxing.
Looking forward to more comments from @hubert.reinterpretcast and @MaskRay.

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

68

Very good suggestion, thanks!

[jhenderson]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

128

One better: const XCOFFYAML::Symbol &YamlSym

[Esme]

Added const

[Esme]

Gentle ping.

[Esme]

Added support for writing symbol name to string table.

[jhenderson]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

14

Nit: LLVM usually has a new line between the licence header and the #include list (based on a quick look at 3 or 4 files).

68

You can drop the parentheses. Do you need to allow space for a null terminator, or does NameSize take that into account?

127–129

How about simply return initRelocations(CurrentOffset);?

294

Nit: the suggested inline edit sounds a bit better to me.

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml

2

Nit: newer tests I'm involved with at least tend to use '##' for comments, to make them stand out from lit and FileCheck lines.

12

Is there an enum you could use to represent this set of flags? It would be preferable to be able write either of the following (flag values are placeholders):

Flags: Exec

or probably

Flags: [Exec, Alloc]

although Flags: 0x20 (or possibly Flags: [0x20]) should probably still be permitted.

13

It seems to me you could get away with much less data in this section (probably a half-dozen bytes)? I don't think this needs to be a real object for this test case? Same below.

[Esme]

Addressed James's comments.

[Esme]

llvm/lib/ObjectYAML/XCOFFEmitter.cpp

68

Well, it seems that NameSize should have taken this into account.

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml

12

Yes, we have the enum SectionTypeFlags. How about marking this as a TODO for follow-up work? Because this will have an impact on other tools, like obj2yaml.

enum SectionTypeFlags : int32_t {
  STYP_PAD = 0x0008,
  STYP_DWARF = 0x0010,
  STYP_TEXT = 0x0020,
  STYP_DATA = 0x0040,
  STYP_BSS = 0x0080,
  STYP_EXCEPT = 0x0100,
  STYP_INFO = 0x0200,
...
};

[Esme]

Used the enum SectionTypeFlags to represent the set of flags.

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml

12

After double checking, this does not seem to affect other tools. Thanks for your input.

[jhenderson]

I've not had time to review test coverage yet, and I don't know XCOFF at all, so can't comment on the correctness either. You'll need others input for the latter.

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml

12

As these are flags, you'll need the ability to specify multiple flags, just like for ELF sections. Looking at this, I'm guessing you don't currently have that option?

[Esme]

Added the ability to specify multiple flags.