This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Allow custom fields for the SHT_UNDEF sections.
ClosedPublic

Authored by grimar on Jul 23 2019, 6:26 AM.

Details

Summary

This is a follow-up refactoring patch for recently
introduced functionality which which reduces the code duplication
and also makes possible to redefine all possible fields of
the first SHT_NULL section (previously it was only possible to set
sh_link and sh_size).

Diff Detail

Event Timeline

grimar created this revision.Jul 23 2019, 6:26 AM
MaskRay added inline comments.Jul 23 2019, 6:57 PM
test/tools/yaml2obj/elf-custom-null-section.yaml
127

Is there a reason for the .foo -> 'foo' change?

SHT_PROGBITS should be aligned

tools/yaml2obj/yaml2elf.cpp
966

What happens if there is non-SHN_UNDEF section with an empty name?

grimar marked 2 inline comments as done.Jul 24 2019, 12:29 AM
grimar added inline comments.
test/tools/yaml2obj/elf-custom-null-section.yaml
127

Is there a reason for the .foo -> 'foo' change?

It's just a different test case. (I added a one or two to the begining, so had to shift down the others).

SHT_PROGBITS should be aligned

I think it is already aligned. We align fields for each section separatelly
taking in account the largest length of the field used.

tools/yaml2obj/yaml2elf.cpp
966

Nothing bad. We have a test case with such situation, btw, it is:
https://github.com/llvm-mirror/llvm/blob/master/test/tools/yaml2obj/unnamed-section.yaml

ELF string table starts from zero byte, i.e. it contains the empty string:
https://github.com/llvm-mirror/llvm/blob/master/lib/MC/StringTableBuilder.cpp#L168
so there is not problems to get the "" string offset.

Initially unnamed-section.yaml seems tested thet we do not crash when have an unnamed section.
When I added the support for zero/undef sections I got complains in this place about "Repeated section name...<empty>"
for unnamed sections. I decided that we can probably just allow them.

MaskRay accepted this revision.Jul 24 2019, 1:22 AM
This revision is now accepted and ready to land.Jul 24 2019, 1:22 AM
jhenderson accepted this revision.Jul 24 2019, 1:27 AM

One nit, otherwise LGTM.

test/tools/yaml2obj/elf-custom-null-section.yaml
131

Should this be %t6, like it was?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 5:15 AM
phosek added a subscriber: phosek.Jul 24 2019, 10:14 AM

The elf-custom-null-section.yaml test is failing on macOS with the following error:

FAIL: LLVM :: tools/yaml2obj/elf-custom-null-section.yaml (32620 of 32657)
******************** TEST 'LLVM :: tools/yaml2obj/elf-custom-null-section.yaml' FAILED ********************
Script:
--
: 'RUN: at line 5';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=1 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp1
: 'RUN: at line 6';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp1 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=DEFAULT
: 'RUN: at line 25';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=2 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp2
: 'RUN: at line 26';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp2 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=DEFAULT
: 'RUN: at line 47';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=3 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp3
: 'RUN: at line 48';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp3 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=REDEFINE
: 'RUN: at line 74';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=4 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp4
: 'RUN: at line 75';   stat /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp3 > /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp.txt
: 'RUN: at line 76';   stat /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp4 >> /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp.txt
: 'RUN: at line 77';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --input-file=/b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp.txt --check-prefix=SIZE
: 'RUN: at line 101';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=5 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp5
: 'RUN: at line 102';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp5 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=OTHER-SECTION
: 'RUN: at line 131';   not /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=6 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp6 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=CASE4
: 'RUN: at line 147';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=7 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp7
: 'RUN: at line 148';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp7 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=DEFAULT
: 'RUN: at line 161';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=8 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp8
: 'RUN: at line 162';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp8 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=MULTIPLE
: 'RUN: at line 188';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/yaml2obj --docnum=9 /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml -o /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp9
: 'RUN: at line 189';   /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/llvm-readelf --sections /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp9 | /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml --check-prefix=OVERRIDE
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/llvm/test/tools/yaml2obj/elf-custom-null-section.yaml:79:9: error: SIZE: expected string not found in input
# SIZE: Size: [[FILESIZE:.*]]
        ^
/b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp.txt:1:1: note: scanning from here
16777218 988254754 -rw-r--r-- 1 chrome-bot staff 0 377 "Jul 24 06:00:40 2019" "Jul 24 06:00:39 2019" "Jul 24 06:00:39 2019" "Jul 24 06:00:39 2019" 4096 8 0 /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp3
^
/b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp.txt:1:171: note: possible intended match here
16777218 988254754 -rw-r--r-- 1 chrome-bot staff 0 377 "Jul 24 06:00:40 2019" "Jul 24 06:00:39 2019" "Jul 24 06:00:39 2019" "Jul 24 06:00:39 2019" 4096 8 0 /b/s/w/ir/k/recipe_cleanup/clangKW_Ksk/llvm_build_dir/test/tools/yaml2obj/Output/elf-custom-null-section.yaml.tmp3
                                                                                                                                                                          ^
jfb reopened this revision.Jul 24 2019, 11:29 AM
jfb added a subscriber: jfb.
This revision is now accepted and ready to land.Jul 24 2019, 11:29 AM
jfb requested changes to this revision.Jul 24 2019, 11:29 AM
This revision now requires changes to proceed.Jul 24 2019, 11:29 AM

So seems stat tool doesn't print "Size: <size>" on MacOS like it does on linux/windows.

Do anyone have an idea how to test that 2 outputs have the same size?

In other our tests we used something like:

# RUN: yaml2obj --docnum=2 %s -o %t2
# RUN: yaml2obj --docnum=3 %s -o %t3
# RUN: od -t x8 -v %t2 > %t.txt
# RUN: od -t x8 -v %t3 >> %t.txt
# RUN: FileCheck %s --input-file=%t.txt --check-prefix=CASE2

# CASE2: [[OFFSET:.*]] fefefefefefefefe
# CASE2: [[FILESIZE:.*]]{{$}}
# CASE2: [[OFFSET]] fefefefefefefefe
# CASE2: [[FILESIZE]]{{$}}

But I found that it is not correct. The value of FILESIZE actually doesn't point to the
last empty line. I tried to use stat because of that instead here.

grimar planned changes to this revision.Jul 25 2019, 12:27 AM

What about if we add something like "FIle size: <x>" field to the llvm-readobj?
Like:

~/LLVM/build/bin/llvm-readobj test.ooo
File: test.ooo
FIle size: XXX
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit
LoadName: <Not found>

MaskRay added a comment.EditedJul 25 2019, 1:15 AM

What about if we add something like "FIle size: <x>" field to the llvm-readobj?
Like:

~/LLVM/build/bin/llvm-readobj test.ooo
File: test.ooo
FIle size: XXX
Format: ELF64-x86-64
Arch: x86_64
AddressSize: 64bit
LoadName: <Not found>

Ah sorry I missed the use of stat in the patch.

I'm not sure if we want to append FIle size: to the llvm-readobj output - it isn't an ELF field. One can do cp /usr/bin/cat /tmp; echo >> /tmp/cat; /tmp/cat # still a cute cat :)

If you just want to check the size of a file, just use ls -l. As POSIX says:

If the -l option is specified, the following information shall be written for files other than character special and block special files:

"%s %u %s %s %u %s %s\n", <file mode>, <number of links>,
    <owner name>, <group name>, <size>, <date and time>,
    <pathname>

You can just use ls -l %t | cut -d ' ' -f 5 to retrieve the size field.

If you just want to check the size of a file, just use ls -l. As POSIX says:

If the -l option is specified, the following information shall be written for files other than character special and block special files:

"%s %u %s %s %u %s %s\n", <file mode>, <number of links>,
    <owner name>, <group name>, <size>, <date and time>,
    <pathname>

You can just use ls -l %t | cut -d ' ' -f 5 to retrieve the size field.

I'll try this, thanks!

If you just want to check the size of a file, just use ls -l. As POSIX says:

If the -l option is specified, the following information shall be written for files other than character special and block special files:

"%s %u %s %s %u %s %s\n", <file mode>, <number of links>,
    <owner name>, <group name>, <size>, <date and time>,
    <pathname>

You can just use ls -l %t | cut -d ' ' -f 5 to retrieve the size field.

I'll try this, thanks!

It doesn't work as is on windows :) The reason seems is an additional space printed after permissions.
I.e. what is 5th field on linux, it is 6th field on windows because of that:

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4
-rw-rw-rw- 1 anon 0 377 2019-07-25 11:50 D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4

The output above ^^^ has double space after "rw-rw-rw-"

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 1
-rw-rw-rw-

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 2

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 3
1

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 4
anon

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 5
0

D:\Work2\llvm>ls -l D:\Work2\llvm\llvm_build\test\tools\yaml2obj\Output\elf-custom-null-section.yaml.tmp4 | cut -d " " -f 6
377

I had to use ls -l %t3 | tr -s ' ' | cut -d ' ' -f 5 finally.

grimar updated this revision to Diff 211699.Jul 25 2019, 2:19 AM
  • Stop using stat.
MaskRay accepted this revision.Jul 25 2019, 2:34 AM

It doesn't work as is on windows :) The reason seems is an additional space printed after permissions. I.e. what is 5th field on linux, it is 6th field on windows because of that:

I had to use ls -l %t3 | tr -s ' ' | cut -d ' ' -f 5 finally.

OK... That Windows ls utility is non-conforming...

This revision was not accepted when it landed; it landed in state Needs Review.Jul 25 2019, 3:20 AM
This revision was automatically updated to reflect the committed changes.

FWIW, rather than messing about with cut and tr, it might have been easier to just use FileCheck directly, i.e. just do ls -l | FileCheck %s, and then construct an appropriate regex to skip the irrelevant bits. I don't think it's a big deal either way, but I would probably find it easier to read, personally.

FWIW, rather than messing about with cut and tr, it might have been easier to just use FileCheck directly, i.e. just do ls -l | FileCheck %s, and then construct an appropriate regex to skip the irrelevant bits. I don't think it's a big deal either way, but I would probably find it easier to read, personally.

I invented the following. But it does not look simpler to me actually...

# RUN: yaml2obj --docnum=4 %s -o %t4
# RUN: ls -l %t3 > %t.txt
# RUN: ls -l %t4 >> %t.txt
# RUN: FileCheck %s --input-file=%t.txt --check-prefix=SIZE

# SIZE: {{^[^ ]*[ ]+[^ ]*[ ][^ ]*[ ][^ ]*[ ]}}[[FILESIZE:[^ ]*]]
# SIZE: [[FILESIZE]]

FWIW, rather than messing about with cut and tr, it might have been easier to just use FileCheck directly, i.e. just do ls -l | FileCheck %s, and then construct an appropriate regex to skip the irrelevant bits. I don't think it's a big deal either way, but I would probably find it easier to read, personally.

I invented the following. But it does not look simpler to me actually...

# RUN: yaml2obj --docnum=4 %s -o %t4
# RUN: ls -l %t3 > %t.txt
# RUN: ls -l %t4 >> %t.txt
# RUN: FileCheck %s --input-file=%t.txt --check-prefix=SIZE

# SIZE: {{^[^ ]*[ ]+[^ ]*[ ][^ ]*[ ][^ ]*[ ]}}[[FILESIZE:[^ ]*]]
# SIZE: [[FILESIZE]]

I think you might be right, fair enough!