This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Fix JSON output for Relocations
ClosedPublic

Authored by paulkirth on Oct 31 2022, 10:26 AM.

Details

Summary

The existing JSON incorrectly outputs line breaks and other invalid JSON.

Example Before this patch:

...
"Relocations":[Section (9) .rela.dyn {
  0xA3B0 R_X86_64_RELATIVE - 0x43D0
  0xA3B8 R_X86_64_RELATIVE - 0x4A30
...

Example After this patch:

...
"Relocations":[Section (9) .rela.dyn {
{"Relocation":{"Offset":41904,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},
"Symbol":{"Value":"","RawValue":0},"Addend":17360}},
{"Relocation":{"Offset":41912,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},
"Symbol":{"Value":"","RawValue":0},"Addend":18992}},
{"Relocation":{"Offset":41920,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},
"Symbol":{"Value":"","RawValue":0},"Addend":17440}},
...

Note there are still issues with the Section, but each Relocation is
now a valid JSON object that can be parsed. Future patches will address
the issues regarding JSON output for the Section.

Diff Detail

Event Timeline

paulkirth created this revision.Oct 31 2022, 10:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
paulkirth requested review of this revision.Oct 31 2022, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 10:26 AM
jhenderson requested changes to this revision.Nov 1 2022, 1:28 AM

The description of this patch really doesn't provide enough info for me. Please could you a) give an actual example of how the JSON is broken, and b) add some tests to show the behaviour change.

Also, the tag on the patch should be llvm-readobj, since that's the tool name.

This revision now requires changes to proceed.Nov 1 2022, 1:28 AM

Today we emit the exact, same text for relocations in both JSON and the LLVM format. The issue is that it isn't valid JSON. In the example below the Section (9) is the first item in the array. But it isnt a key, and isn't an object. To make matters worse, the newlines are hard coded, so even if you don't want pretty printing, you end up with JSON filled with newlines. If you compare the printRelRelaReloc() implementation for the JSONELFDumper and the LLVMELFDumper, you'll see that the JSON one is basically identical to the LLVM style when the expand relocs option is set. While that makes the output more verbose, it's also much more correct, though there are still other issues(like the newlines from the Section printer ...).

Example output:

  • Before
[{"FileSummary":{"File":"a.out","Format":"elf64-x86-64","Arch":"x86_64","AddressSize":"64bit","LoadName":"<Not found>"},"Relocations":[Section (9) .rela.dyn {
  0xA3B0 R_X86_64_RELATIVE - 0x43D0
  0xA3B8 R_X86_64_RELATIVE - 0x4A30
  0xA3C0 R_X86_64_RELATIVE - 0x4420
  0xA3C8 R_X86_64_RELATIVE - 0x1622
  0xA3D0 R_X86_64_RELATIVE - 0x16B0
  0xA3D8 R_X86_64_RELATIVE - 0x18A1
  0xA3E0 R_X86_64_RELATIVE - 0x1955
  0xB5D0 R_X86_64_RELATIVE - 0xB5D0
  0xB5D8 R_X86_64_RELATIVE - 0xB8A0
  0xB5E0 R_X86_64_RELATIVE - 0x118A0
  0xB5F0 R_X86_64_RELATIVE - 0x86C0
  0xB5F8 R_X86_64_RELATIVE - 0x7A60
  0xB600 R_X86_64_RELATIVE - 0x7CC0
  0xB608 R_X86_64_RELATIVE - 0x87F0
  0xB610 R_X86_64_RELATIVE - 0x8810
  0xB618 R_X86_64_RELATIVE - 0x8890
  0xB6D8 R_X86_64_RELATIVE - 0x4490
  0xB708 R_X86_64_RELATIVE - 0x4730
  0xB738 R_X86_64_RELATIVE - 0x4760
  0xB768 R_X86_64_RELATIVE - 0x4790
  0xB798 R_X86_64_RELATIVE - 0x47C0
  0xB7C8 R_X86_64_RELATIVE - 0x4860
  0xB7F8 R_X86_64_RELATIVE - 0x4900
  0xB828 R_X86_64_RELATIVE - 0x49C0
  0xB858 R_X86_64_RELATIVE - 0x49E0
  0xB888 R_X86_64_RELATIVE - 0x4A00
  0xA588 R_X86_64_GLOB_DAT __libc_start_main@GLIBC_2.34 0x0
  0xA590 R_X86_64_GLOB_DAT __gmon_start__ 0x0
  0xA598 R_X86_64_GLOB_DAT __register_frame_info 0x0
  0xA5A0 R_X86_64_GLOB_DAT __cxa_finalize@GLIBC_2.2.5 0x0
  0xA5A8 R_X86_64_GLOB_DAT __deregister_frame_info 0x0
  0xA5B0 R_X86_64_GLOB_DAT free@GLIBC_2.2.5 0x0
  0xA5B8 R_X86_64_GLOB_DAT stderr@GLIBC_2.2.5 0x0
}
Section (10) .rela.plt {
  0x118B8 R_X86_64_JUMP_SLOT __register_frame_info 0x0
  0x118C0 R_X86_64_JUMP_SLOT __cxa_finalize@GLIBC_2.2.5 0x0
  0x118C8 R_X86_64_JUMP_SLOT __deregister_frame_info 0x0
  0x118D0 R_X86_64_JUMP_SLOT strlen@GLIBC_2.2.5 0x0
  0x118D8 R_X86_64_JUMP_SLOT rand@GLIBC_2.2.5 0x0
  0x118E0 R_X86_64_JUMP_SLOT printf@GLIBC_2.2.5 0x0
  0x118E8 R_X86_64_JUMP_SLOT free@GLIBC_2.2.5 0x0
  0x118F0 R_X86_64_JUMP_SLOT calloc@GLIBC_2.2.5 0x0
  0x118F8 R_X86_64_JUMP_SLOT malloc@GLIBC_2.2.5 0x0
  0x11900 R_X86_64_JUMP_SLOT memcpy@GLIBC_2.14 0x0
  0x11908 R_X86_64_JUMP_SLOT fprintf@GLIBC_2.2.5 0x0
  0x11910 R_X86_64_JUMP_SLOT getpid@GLIBC_2.2.5 0x0
  0x11918 R_X86_64_JUMP_SLOT snprintf@GLIBC_2.2.5 0x0
  0x11920 R_X86_64_JUMP_SLOT getenv@GLIBC_2.2.5 0x0
  0x11928 R_X86_64_JUMP_SLOT strcmp@GLIBC_2.2.5 0x0
  0x11930 R_X86_64_JUMP_SLOT __strdup@GLIBC_2.2.5 0x0
  0x11938 R_X86_64_JUMP_SLOT getpagesize@GLIBC_2.2.5 0x0
  0x11940 R_X86_64_JUMP_SLOT __errno_location@GLIBC_2.2.5 0x0
  0x11948 R_X86_64_JUMP_SLOT strerror@GLIBC_2.2.5 0x0
  0x11950 R_X86_64_JUMP_SLOT fopen@GLIBC_2.2.5 0x0
  0x11958 R_X86_64_JUMP_SLOT fileno@GLIBC_2.2.5 0x0
  0x11960 R_X86_64_JUMP_SLOT ftruncate@GLIBC_2.2.5 0x0
  0x11968 R_X86_64_JUMP_SLOT munmap@GLIBC_2.2.5 0x0
  0x11970 R_X86_64_JUMP_SLOT fseek@GLIBC_2.2.5 0x0
  0x11978 R_X86_64_JUMP_SLOT strtol@GLIBC_2.2.5 0x0
  0x11980 R_X86_64_JUMP_SLOT fclose@GLIBC_2.2.5 0x0
  0x11988 R_X86_64_JUMP_SLOT fflush@GLIBC_2.2.5 0x0
  0x11990 R_X86_64_JUMP_SLOT fwrite@GLIBC_2.2.5 0x0
  0x11998 R_X86_64_JUMP_SLOT ftell@GLIBC_2.2.5 0x0
  0x119A0 R_X86_64_JUMP_SLOT mmap@GLIBC_2.2.5 0x0
  0x119A8 R_X86_64_JUMP_SLOT setenv@GLIBC_2.2.5 0x0
  0x119B0 R_X86_64_JUMP_SLOT strncpy@GLIBC_2.2.5 0x0
  0x119B8 R_X86_64_JUMP_SLOT memset@GLIBC_2.2.5 0x0
  0x119C0 R_X86_64_JUMP_SLOT mkdir@GLIBC_2.2.5 0x0
  0x119C8 R_X86_64_JUMP_SLOT uname@GLIBC_2.2.5 0x0
  0x119D0 R_X86_64_JUMP_SLOT fcntl@GLIBC_2.2.5 0x0
  0x119D8 R_X86_64_JUMP_SLOT open@GLIBC_2.2.5 0x0
  0x119E0 R_X86_64_JUMP_SLOT fdopen@GLIBC_2.2.5 0x0
  0x119E8 R_X86_64_JUMP_SLOT strchr@GLIBC_2.2.5 0x0
  0x119F0 R_X86_64_JUMP_SLOT strrchr@GLIBC_2.2.5 0x0
  0x119F8 R_X86_64_JUMP_SLOT prctl@GLIBC_2.2.5 0x0
  0x11A00 R_X86_64_JUMP_SLOT madvise@GLIBC_2.2.5 0x0
  0x11A08 R_X86_64_JUMP_SLOT __cxa_atexit@GLIBC_2.2.5 0x0
}
]}]
  • After
[{"FileSummary":{"File":"a.out","Format":"elf64-x86-64","Arch":"x86_64","AddressSize":"64bit","LoadName":"<Not found>"},"Relocations":[Section (9) .rela.dyn {
{"Relocation":{"Offset":41904,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":17360}},{"Relocation":{"Offset":41912,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18992}},{"Relocation":{"Offset":41920,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":17440}},{"Relocation":{"Offset":41928,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":5666}},{"Relocation":{"Offset":41936,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":5808}},{"Relocation":{"Offset":41944,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":6305}},{"Relocation":{"Offset":41952,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":6485}},{"Relocation":{"Offset":46544,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":46544}},{"Relocation":{"Offset":46552,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":47264}},{"Relocation":{"Offset":46560,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":71840}},{"Relocation":{"Offset":46576,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":34496}},{"Relocation":{"Offset":46584,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":31328}},{"Relocation":{"Offset":46592,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":31936}},{"Relocation":{"Offset":46600,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":34800}},{"Relocation":{"Offset":46608,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":34832}},{"Relocation":{"Offset":46616,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":34960}},{"Relocation":{"Offset":46808,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":17552}},{"Relocation":{"Offset":46856,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18224}},{"Relocation":{"Offset":46904,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18272}},{"Relocation":{"Offset":46952,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18320}},{"Relocation":{"Offset":47000,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18368}},{"Relocation":{"Offset":47048,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18528}},{"Relocation":{"Offset":47096,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18688}},{"Relocation":{"Offset":47144,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18880}},{"Relocation":{"Offset":47192,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18912}},{"Relocation":{"Offset":47240,"Type":{"Value":"R_X86_64_RELATIVE","RawValue":8},"Symbol":{"Value":"","RawValue":0},"Addend":18944}},{"Relocation":{"Offset":42376,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"__libc_start_main@GLIBC_2.34","RawValue":1},"Addend":0}},{"Relocation":{"Offset":42384,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"__gmon_start__","RawValue":2},"Addend":0}},{"Relocation":{"Offset":42392,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"__register_frame_info","RawValue":3},"Addend":0}},{"Relocation":{"Offset":42400,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"__cxa_finalize@GLIBC_2.2.5","RawValue":4},"Addend":0}},{"Relocation":{"Offset":42408,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"__deregister_frame_info","RawValue":5},"Addend":0}},{"Relocation":{"Offset":42416,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"free@GLIBC_2.2.5","RawValue":9},"Addend":0}},{"Relocation":{"Offset":42424,"Type":{"Value":"R_X86_64_GLOB_DAT","RawValue":6},"Symbol":{"Value":"stderr@GLIBC_2.2.5","RawValue":13},"Addend":0}}}
Section (10) .rela.plt {
,{"Relocation":{"Offset":71864,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__register_frame_info","RawValue":3},"Addend":0}},{"Relocation":{"Offset":71872,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__cxa_finalize@GLIBC_2.2.5","RawValue":4},"Addend":0}},{"Relocation":{"Offset":71880,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__deregister_frame_info","RawValue":5},"Addend":0}},{"Relocation":{"Offset":71888,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strlen@GLIBC_2.2.5","RawValue":6},"Addend":0}},{"Relocation":{"Offset":71896,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"rand@GLIBC_2.2.5","RawValue":7},"Addend":0}},{"Relocation":{"Offset":71904,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"printf@GLIBC_2.2.5","RawValue":8},"Addend":0}},{"Relocation":{"Offset":71912,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"free@GLIBC_2.2.5","RawValue":9},"Addend":0}},{"Relocation":{"Offset":71920,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"calloc@GLIBC_2.2.5","RawValue":10},"Addend":0}},{"Relocation":{"Offset":71928,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"malloc@GLIBC_2.2.5","RawValue":11},"Addend":0}},{"Relocation":{"Offset":71936,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"memcpy@GLIBC_2.14","RawValue":12},"Addend":0}},{"Relocation":{"Offset":71944,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fprintf@GLIBC_2.2.5","RawValue":14},"Addend":0}},{"Relocation":{"Offset":71952,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"getpid@GLIBC_2.2.5","RawValue":15},"Addend":0}},{"Relocation":{"Offset":71960,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"snprintf@GLIBC_2.2.5","RawValue":16},"Addend":0}},{"Relocation":{"Offset":71968,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"getenv@GLIBC_2.2.5","RawValue":17},"Addend":0}},{"Relocation":{"Offset":71976,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strcmp@GLIBC_2.2.5","RawValue":18},"Addend":0}},{"Relocation":{"Offset":71984,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__strdup@GLIBC_2.2.5","RawValue":19},"Addend":0}},{"Relocation":{"Offset":71992,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"getpagesize@GLIBC_2.2.5","RawValue":20},"Addend":0}},{"Relocation":{"Offset":72000,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__errno_location@GLIBC_2.2.5","RawValue":21},"Addend":0}},{"Relocation":{"Offset":72008,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strerror@GLIBC_2.2.5","RawValue":22},"Addend":0}},{"Relocation":{"Offset":72016,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fopen@GLIBC_2.2.5","RawValue":23},"Addend":0}},{"Relocation":{"Offset":72024,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fileno@GLIBC_2.2.5","RawValue":24},"Addend":0}},{"Relocation":{"Offset":72032,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"ftruncate@GLIBC_2.2.5","RawValue":25},"Addend":0}},{"Relocation":{"Offset":72040,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"munmap@GLIBC_2.2.5","RawValue":26},"Addend":0}},{"Relocation":{"Offset":72048,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fseek@GLIBC_2.2.5","RawValue":27},"Addend":0}},{"Relocation":{"Offset":72056,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strtol@GLIBC_2.2.5","RawValue":28},"Addend":0}},{"Relocation":{"Offset":72064,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fclose@GLIBC_2.2.5","RawValue":29},"Addend":0}},{"Relocation":{"Offset":72072,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fflush@GLIBC_2.2.5","RawValue":30},"Addend":0}},{"Relocation":{"Offset":72080,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fwrite@GLIBC_2.2.5","RawValue":31},"Addend":0}},{"Relocation":{"Offset":72088,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"ftell@GLIBC_2.2.5","RawValue":32},"Addend":0}},{"Relocation":{"Offset":72096,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"mmap@GLIBC_2.2.5","RawValue":33},"Addend":0}},{"Relocation":{"Offset":72104,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"setenv@GLIBC_2.2.5","RawValue":34},"Addend":0}},{"Relocation":{"Offset":72112,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strncpy@GLIBC_2.2.5","RawValue":35},"Addend":0}},{"Relocation":{"Offset":72120,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"memset@GLIBC_2.2.5","RawValue":36},"Addend":0}},{"Relocation":{"Offset":72128,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"mkdir@GLIBC_2.2.5","RawValue":37},"Addend":0}},{"Relocation":{"Offset":72136,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"uname@GLIBC_2.2.5","RawValue":38},"Addend":0}},{"Relocation":{"Offset":72144,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fcntl@GLIBC_2.2.5","RawValue":39},"Addend":0}},{"Relocation":{"Offset":72152,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"open@GLIBC_2.2.5","RawValue":40},"Addend":0}},{"Relocation":{"Offset":72160,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"fdopen@GLIBC_2.2.5","RawValue":41},"Addend":0}},{"Relocation":{"Offset":72168,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strchr@GLIBC_2.2.5","RawValue":42},"Addend":0}},{"Relocation":{"Offset":72176,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"strrchr@GLIBC_2.2.5","RawValue":43},"Addend":0}},{"Relocation":{"Offset":72184,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"prctl@GLIBC_2.2.5","RawValue":44},"Addend":0}},{"Relocation":{"Offset":72192,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"madvise@GLIBC_2.2.5","RawValue":45},"Addend":0}},{"Relocation":{"Offset":72200,"Type":{"Value":"R_X86_64_JUMP_SLOT","RawValue":7},"Symbol":{"Value":"__cxa_atexit@GLIBC_2.2.5","RawValue":46},"Addend":0}}}
]}]%
paulkirth updated this revision to Diff 481119.Dec 7 2022, 5:37 PM
paulkirth retitled this revision from [readobj] Fix JSON output for Relocations to [llvm-readobj] Fix JSON output for Relocations.
paulkirth edited the summary of this revision. (Show Details)

Rebase and update summary with example

Thanks for the explanation. With the examples, I see your problem.

Unfortunately, I don't have the time to review this properly today (plus it needs tests anyway to show that the code behaves as expected), and as noted elsewhere, I'm away for 6 weeks after today, so it'll be late January or even February before I get as far as reviewing this. Hopefully somebody else can pick this up (@MaskRay?).

paulkirth updated this revision to Diff 483647.Dec 16 2022, 1:17 PM
  • Avoid code duplication using smaller helper functions
  • Simplify testing
paulkirth updated this revision to Diff 484097.Dec 19 2022, 3:00 PM

Fix Formatting

As with the previous patch, I'd look at modifying https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/relocations.test (and possibly other tests if that test doesn't provide sufficient coverage). Otherwise, this looks reasonable to me.

paulkirth updated this revision to Diff 500339.Feb 24 2023, 6:29 PM

Rebase and address comments.

  • update test code w/ JSON sections

I think you may have misinterpreted my previous comments about testing a little. I don't think you need to test all the details that you are now testing in this patch. In particular, the error cases are from common code, and don't really need testing for JSON, I'd have thought (you may have one test for error behaviour, to show the error message is not part of the JSON format, if you want, I guess). It's probably sufficient to show that relocations are printed in "expanded" form, whether or not --expand-relocs is used for JSON output. Up to you, but I'm not convinced you even need to test the 32-bit versus 64-bit differences.

llvm/test/tools/llvm-readobj/ELF/relocations.test
12

Nit: please don't add this extra blank line.

113
392–393

Nit: only one blank line please.

422

Nit: only one blank line please.

469
557

Ditto.

paulkirth updated this revision to Diff 502194.Mar 3 2023, 11:04 AM
paulkirth marked 6 inline comments as done.

Remove extranious testing for common code, and reduce reduncancy.

  • Remove extra lines.

I think this patchset is more what you had in mind w.r.t. testing, but please let me know if I've swung back too far in the other direction.

paulkirth updated this revision to Diff 502195.Mar 3 2023, 11:12 AM

Fix typo in RUN line

paulkirth updated this revision to Diff 502226.Mar 3 2023, 12:39 PM

Remove JSON test for warnings since it doesn't add much. Later changes will
also make the JSON output around the warning slightly different, so its better
to avoid it altogether.

jhenderson accepted this revision.Mar 17 2023, 1:24 AM

LGTM, with some nits.

llvm/test/tools/llvm-readobj/ELF/relocations.test
113

Nit isn't fully addressed (missing full stop at end of comment).

llvm/tools/llvm-readobj/ELFDumper.cpp
6759–6760

Do these really make sense as const? I'm pretty sure StringRef isn't usually a const since you can't modify the string.

6771

Ditto.

This revision is now accepted and ready to land.Mar 17 2023, 1:24 AM
paulkirth updated this revision to Diff 506225.Mar 17 2023, 4:46 PM
paulkirth marked 3 inline comments as done.
paulkirth edited the summary of this revision. (Show Details)

Address comments.

  • Add missing . in test.
  • Remove const qualifier from StringRef.
This revision was landed with ongoing or failed builds.Mar 17 2023, 4:58 PM
This revision was automatically updated to reflect the committed changes.