Page MenuHomePhabricator

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

Authored by Esme on Jan 26 2021, 9:31 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Mar 30 2021, 12:23 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
128

One better: const XCOFFYAML::Symbol &YamlSym

Esme updated this revision to Diff 334069.Mar 30 2021, 1:33 AM

Added const

Esme added a comment.Apr 7 2021, 7:21 PM

Gentle ping.

Esme updated this revision to Diff 337365.Apr 14 2021, 1:22 AM

Added support for writing symbol name to string table.

jhenderson added inline comments.Apr 21 2021, 3:13 AM
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 updated this revision to Diff 340407.Apr 25 2021, 5:54 PM
Esme edited the summary of this revision. (Show Details)
Esme removed a reviewer: steven.zhang.

Addressed James's comments.

Esme added inline comments.Apr 25 2021, 5:54 PM
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 updated this revision to Diff 340421.Apr 25 2021, 8:19 PM

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.

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 updated this revision to Diff 341089.Apr 28 2021, 12:59 AM

Added the ability to specify multiple flags.

I've taken another look and think you need a fair bit more testing.

  1. You need to show that all the fields can be specified explicitly. Currently, you only do it for a subset of the fields.
  2. You probably should show that all the flags are supported.
  3. You need to test your error paths, to show that the errors are reported properly.
  4. If you don't need the string table support in the initial patch (and by my understanding you don't), you can move all the related logic into another indepenedent patch, and then ensure it is properly tested there.
  5. I think you need testing for undef and abs symbols too,
  6. I think you need testing for non data/text/bss sections (with their zero addresses).
llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml
1
3–4

You can do this all at once.

Esme updated this revision to Diff 346328.May 18 2021, 8:16 PM

Addressed comments.

Addressed comments.

Hi @Esme. Have you attempted to address all my comments? It doesn't look like you have. If you update a patch and haven't finished addressing all comments, please make it clear from the comment what you still plan to do.

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

Be consistent: above you've quoted the section data, and here you haven't. Pick one style (I'd recommend quoting).

25–26

Any particular reason these aren't all one section?

llvm/test/tools/yaml2obj/XCOFF/full-contents.yaml
67 ↗(On Diff #346328)

Why are there blank lines here and below? Does this test actually pass? I was under the impression that CHECK-NEXT: without any contents was an error.

Esme updated this revision to Diff 347565.Mon, May 24, 8:46 PM
Esme added a comment.Mon, May 24, 8:47 PM

I've taken another look and think you need a fair bit more testing.

  1. You need to show that all the fields can be specified explicitly. Currently, you only do it for a subset of the fields.
  2. You probably should show that all the flags are supported.
  3. You need to test your error paths, to show that the errors are reported properly.
  4. If you don't need the string table support in the initial patch (and by my understanding you don't), you can move all the related logic into another indepenedent patch, and then ensure it is properly tested there.
  5. I think you need testing for undef and abs symbols too,
  6. I think you need testing for non data/text/bss sections (with their zero addresses).

Thanks! @jhenderson

  1. The test file llvm/test/tools/yaml2obj/XCOFF/full-contents.yaml is added.
  2. Addressed.
  3. There are two types of errors, one is exceeding the maximum size/index, and the other is writing with offset overlap/redundant data. For the first type of error, it is difficult to write such a large file to show the error message; and for the second type, in order to allow users to write explicitly specifies values (allowing for invalid values), our offsets are derived from the contents, therefore these errors should not occur unless the yaml2obj itself has bugs.
  4. I think we should support the string table in the patch, and I have added the long symbol name to test it.
  5. Addressed.
  6. Addressed.
  1. There are two types of errors, one is exceeding the maximum size/index, and the other is writing with offset overlap/redundant data. For the first type of error, it is difficult to write such a large file to show the error message; and for the second type, in order to allow users to write explicitly specifies values (allowing for invalid values), our offsets are derived from the contents, therefore these errors should not occur unless the yaml2obj itself has bugs.

Fair enough. Could you at least test manually the max section limit case. It shouldn't be too hard to do (write a small script to generate such a case), please?

  1. I think we should support the string table in the patch, and I have added the long symbol name to test it.

Why do you think string table support should be in this patch? Why not split the patch up into smaller pieces to make it easier for reviewers to review it? There's no need for the behaviour to be fully functional in the first case, especially as by my understanding not all XCOFF objects have string tables.

Esme updated this revision to Diff 347855.Wed, May 26, 12:05 AM

Leave the StringTable as a follow-up work.

Esme added a comment.Wed, May 26, 12:06 AM

Could you at least test manually the max section limit case.

I tested the case with a python script generating the yaml doc, and the error message was output as expected.

jhenderson accepted this revision.Wed, May 26, 12:52 AM

LGTM from a yaml2obj standpoint, but it would be a good idea to rope in someone with XCOFF experience to confirm they are happy with the file format writing before committing this.

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

Does this symbol test anything unique any more, or can we drop it?

This revision is now accepted and ready to land.Wed, May 26, 12:52 AM
Esme added a comment.Thu, May 27, 8:27 PM

Hi @jasonliu @DiggerLin @sfertile @shchenz @hubert.reinterpretcast, I would appreciate it if you could review this patch and check if the file format is written properly in your opinion. Thanks in advance!

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

It was used to test StringTable before, but now it's useless. I will drop this in next update.

Esme updated this revision to Diff 348719.Sun, May 30, 7:25 PM
  1. Drop the useless Symbol test.
  2. Add normalization/denormalization for the Section Flags set.
shchenz added inline comments.Sun, May 30, 8:18 PM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
242

We don't have to call write_zeros if PaddingSize is 0?

265

What about a symbol that does not have belonged sections? For example undefined external symbols?

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

Is it ok that the relocation address is not in the range of .data section?

20

use another section name that is not DWARF specific?

llvm/test/tools/yaml2obj/XCOFF/full-contents.yaml
36 ↗(On Diff #347855)

Same as above

Esme updated this revision to Diff 348738.Mon, May 31, 12:58 AM
  1. Addressed Zheng's comments.
  2. Set more fields optional and modified the correspond testing.
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
265
Specifies a section number associated with one of the following symbols:
-2
Specifies N_DEBUG, a special symbolic debugging symbol.
-1
Specifies N_ABS, an absolute symbol. The symbol has a value but is not relocatable.
0
Specifies N_UNDEF, an undefined external symbol.
Any other value
Specifies the section number where the symbol was defined.

As the spec, there are 3 reserved section numbers for these symbols not defined in sections. The SectionName in the symbol entry is required now, however, we can also set the field optional and use N_UNDEF (maybe...) as the default section. Hmm... I am not sure which strategy is more reasonable. What do you think?

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

In general, yaml2obj will not report an error even though the user explicitly specifies an invalid values, which allows for things like error handling testing.

Also after getting more familiar with yaml2obj, I think it's reasonable to set more fields to optional, and these omitted values will be filled in with the default zero or values derived from contents. So I set the relocation address optional now and 0 is the default value for it.

shchenz added inline comments.Mon, May 31, 1:38 AM
llvm/lib/ObjectYAML/XCOFFEmitter.cpp
265

I think SectionName for reserved section number N_UNDEF is not defined because we don't need to do it. We only need a section number. So making SectionName attribute optional for a symbol makes more sense to me. You can not say that a section with section name N_UNDEF is the N_UNDEF(section number == 0) section. We can define a text section named by N_UNDEF by using section attribute like:

__attribute__((section ("N_UNDEF")))  int foo2(void)
{
}
llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml
17

Do you mean we setting the address as 0x3A on purpose? The address of the .data section is 0x8 and its size is 0x8, so if the relocation is valid, the address the relocation entry wants to resolve should be in 0x8 and 0x10.

Esme updated this revision to Diff 348754.Mon, May 31, 2:37 AM

Address comment.

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

I just set a random value, as this value isn't relevant for testing purposes here. Use a valid address now.

shchenz accepted this revision.Tue, Jun 1, 5:32 AM

LGTM too. Please wait for several days in case other reviewers have comments. Thanks for enabling this tool for AIX.

This revision was landed with ongoing or failed builds.Sun, Jun 6, 9:15 PM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
llvm/utils/gn/secondary/llvm/lib/ObjectYAML/BUILD.gn
29

(FYI, you don't need to update the files in llvm/utils/gn when you add files. Those are unsupported build files, and also for simple changes like this they are updated automatically based on the CMakeLists.txt files by a bot.

If you _do_ update them, please add a trailing , -- but it's better to not update them since it's less work for you and the bot will update them correctly :) )

This patch introduce https://lab.llvm.org/buildbot/#/builders/85/builds/4886

llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp:67:16: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../x86_64-linux-gnu/include/string.h:43:28: note: nonnull attribute specified here
    #0 0x45c3df in (anonymous namespace)::XCOFFWriter::writeXCOFF() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
    #1 0x45b0b1 in llvm::yaml::yaml2xcoff(llvm::XCOFFYAML::Object&, llvm::raw_ostream&, llvm::function_ref<void (llvm::Twine const&)>) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp:311:17
    #2 0x30a181 in llvm::yaml::convertYAML(llvm::yaml::Input&, llvm::raw_ostream&, llvm::function_ref<void (llvm::Twine const&)>, unsigned int, unsigned long) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/yaml2obj.cpp:48:14
    #3 0x306322 in main /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/yaml2obj/yaml2obj.cpp:136:8
    #4 0x7f38e52b709a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #5 0x2eafd9 in _start (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/yaml2obj+0x2eafd9)

CC @eugenis

Esme added a comment.Wed, Jun 9, 1:45 AM

This patch introduce https://lab.llvm.org/buildbot/#/builders/85/builds/4886

llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp:67:16: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../x86_64-linux-gnu/include/string.h:43:28: note: nonnull attribute specified here
    #0 0x45c3df in (anonymous namespace)::XCOFFWriter::writeXCOFF() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
    #1 0x45b0b1 in llvm::yaml::yaml2xcoff(llvm::XCOFFYAML::Object&, llvm::raw_ostream&, llvm::function_ref<void (llvm::Twine const&)>) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/XCOFFEmitter.cpp:311:17
    #2 0x30a181 in llvm::yaml::convertYAML(llvm::yaml::Input&, llvm::raw_ostream&, llvm::function_ref<void (llvm::Twine const&)>, unsigned int, unsigned long) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjectYAML/yaml2obj.cpp:48:14
    #3 0x306322 in main /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/yaml2obj/yaml2obj.cpp:136:8
    #4 0x7f38e52b709a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #5 0x2eafd9 in _start (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/yaml2obj+0x2eafd9)

CC @eugenis

The issue has been solved by rG310d2b4957c8, thanks.

llvm/utils/gn/secondary/llvm/lib/ObjectYAML/BUILD.gn
29

Thanks for the information. I will not update such file in future patches.