This allows us better readability and
compatibility with GNU objdump prints.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for the patch!
It looks like you haven't run the tests. Please make sure to run the check-all target, to run all the tests. In particular, I expect to see many tests that will fail, because they do not expect the extra line for the headers. Also, please make sure to update the llvm/test/tools/llvm-objdump/elf-relocations.test to check this works. You will need to add a 32-bit test case probably, to show that the 16 versus 8 space padding decision is made correctly. If you need help writing these tests, please let me know, and I'll be happy to help out.
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
1648–1649 | LLVM's normal variable naming style is UpperCamelCase, so this should be OffsetPadding, and TypePadding. |
I've updated the relocations-elf.test file with the new headers, they seem to recognize the headers quite well, however, the relocations on my machine come out different than the test expects, so the other parts of the test fail...
Are there any requirements for the machine I am running my test on?
I'm not aware of any specific requirements for this test. How are you running the test? Have you remembered to build the other tools in LLVM that it uses? (yaml2obj, llvm-objdump, FileCheck?). What is the output you get when you run the test?
OK I was mistaken, sorry about that, I've fixed the tests so they all contain the headers, 2 notes on that:
- it seems that the tests do not really test white spaces for some reason (I tried randomly changing the padding in the test to check that, as long as the right words were written in the right order all was well) does it make any sense to you?
- The web assembly test already contains a 32-bit address, would you like me to add a specific test case for explicit 32-bit for this matter?
Please could you upload the latest patch including your test changes, so that I can review them.
- it seems that the tests do not really test white spaces for some reason (I tried randomly changing the padding in the test to check that, as long as the right words were written in the right order all was well) does it make any sense to you?
By default, FileCheck ignores leading and trailing whitespace on the check pattern's line and canonicalises all other whitespace to a single space. Additionally, it only does partial matches by default (i.e. if the CHECK string is "foo bar", it will succeed if passed "this string contains 'foo bar'"). In most cases, whitespace isn't really important. However, in some cases, like this change, we want to test that the formatting is correct. This is important because some people parse the output and expect exact numbers of spaces, and also, it ensures the output is more readable. In cases where whitespace is important, you can use FileCheck's --strict-whitespace option, which doesn't do the whitespace canonicalisation, although it does still strip trailing and leading spaces from the line. That's probably sufficient for your use-case here. (For reference, you can use --match-full-lines to match the entire line, rather than a partial match; when combined with --strict-whitespace, it also makes sure leading and trailing whitsepace matches). It might be helpful to refer to the FileCheck documentation.
- The web assembly test already contains a 32-bit address, would you like me to add a specific test case for explicit 32-bit for this matter?
I think adding an ELF 32-bit case would be useful. I'd add it to the same file as the existing ELF 64 case. However, I don't think it is strictly needed, so don't feel obliged to.
I think you can only add --strict-whitespaces to relocations-elf.test. No need to touch and change another tests probably,
it is enough to test the paddings in one place once and not spread this check everywhere (if there is no valid reason).
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
148 | Missing a full stop. | |
150 | I'd 32BITPADDING -> ELF32 | |
153 | I think you either need to use --match-full-lines or use {{^}} which checks the begining of the line: # 32BITPADDING: {{^}}OFFSET TYPE VALUE because currently you do not check the paddings at the begining of the lines. | |
167 | I see that you've basing on the YAML from above, but we usually do not | |
191 | It is strange to see SHT_RELA section in a ELFCLASS32 object. | |
211 | It seems enough to have one local and one global symbol here. | |
218 | It is generally a good practice to minimize the YAML used. I think you can use something like this: --- !ELF FileHeader: Class: ELFCLASS32 Data: ELFDATA2LSB Type: ET_REL Machine: EM_386 Sections: - Name: .text Type: SHT_PROGBITS - Name: .rel.text Type: SHT_REL Info: .text Relocations: - Offset: 0x1 Symbol: global Type: R_386_PC32 - Offset: 0x2 Symbol: loc Type: R_386_32 Symbols: - Name: loc - Name: global Binding: STB_GLOBAL |
Looking again, it seems it is OK to keep --strict-whitespace in WebAssembly/relocations.test as it is a generic test for relocations output for WASM.
I do not think we should add --strict-whitespace to COFF tests here as they are testing different things, not just regular output.
(i.e. it is probably not a proper place, they might want to add a generic test probably).
I wonder what other people thing about this though.
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
191 | (I mean it has SHT_REL, what is unnatural for ELFCLASS64 in the same way like SHT_RELA is unnatural for ELFCLASS32). |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
148 | What's full stop? |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
148 | The symbol . used in writing at the end of a sentence. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
148 | I'd change "padding" to "formatting" as it also shows that the offset fieds are of the appropriate size. | |
191 | I'd keep both SHT_REL and SHT_RELA sections. The gABI doesn't limit these sections to architectures of one or other size (although some processor supplements do), so we should show we can handle all situations. I know at least one system I worked on used SHT_REL originally and then later swapped to SHT_RELA. | |
llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test | ||
9 | I'm not sure you need this additional test case here. I don't think it adds anything above the line above. |
llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test | ||
---|---|---|
9 | Checking strict-whitespaces on FMT kept failing and it didn't really matter on the padding change |
Updated required changes, removed unnecessary additions of --strict-whitespace, I did not change the content of the ELF32 test because there was no conclusive decision to whether I should or shouldn't remove the extra relocations on that test.
llvm/test/tools/llvm-objdump/WebAssembly/relocations.test | ||
---|---|---|
12–14 | As a slight readability suggestion, consider doing the following: ; CHECK:RELOCATION RECORDS FOR [CODE]: This allows for the ':' characters and therefore the actual checked-for text to line up. Same applies a few lines below. | |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
7–14 | Same comment as the WASM test. | |
150 | ELF32 is the canonical naming style for 32-bit ELF (and ELF64 for 64-bit ELF). Please use that, not 32ELF, as @grimar originally asked for. | |
152 | Same comment as other tests. | |
191 | @grimar, are you happy with my suggestion of keeping both? |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
191 | Ah, yes, sorry for not adding an explicit comment about it. |
and allows for better
Looks like the summary misses some sentence.
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
191 | lld accepts both SHT_REL and SHT_RELA input. It produces either SHT_REL or SHT_RELA depending on config->isRela (like a target preference). powerpc and riscv32 are two 32-bit targets using RELA. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
159 | Delete "!FileHeader". It serves no purpose. You might want to do it for the other doc in this file in a separate change too. | |
173 | Nit: Add some spaces to make the values all line up, i.e. - Offset: 0x1 Symbol: global Type: R_386_32 Same goes below for the other relocations. | |
194 |
This still hasn't been addressed. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
159 | You've deleted too much. I only wanted the bit after the ':' deleted. Please reinstate the FileHeader: part. Does this work in its current state? That surprises me if it does. | |
llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test | ||
18 | Here's an example of what a FileHeader declaration should look like. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
184 | Why there is no relocation for local here? |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
184 | Since you asked for a minimal YAML, I don't mind adding one. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
184 | The YAML description I've suggested earlier tested both local and global: Relocations: - Offset: 0x1 Symbol: global Type: R_386_PC32 - Offset: 0x2 Symbol: loc Type: R_386_32 Also, I see no reason to test both local and global symbol for SHT_RELA/SHT_REL in the ELFCLASS64 test, So yes, while we test both local and global everywhere, this place should not be an exception I think. |
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
184 | Fixed :) |
I have no more comments except the single nit.
Perhaps @jhenderson should give a final approve for this.
llvm/test/tools/llvm-objdump/relocations-elf.test | ||
---|---|---|
157 | I'd use R_386_PC32 here to be consistent with local above. |
Thank you all for your help with this patch, I do not have permissions to merge, how can I get this merged?
I'll take a look at committing this one for you. What email address and author values do you want me to include for your git commit message?
Once this has landed, you may want to consider requesting commit access (https://llvm.org/docs/DeveloperPolicy.html#new-contributors).
Hi @liadz0rz,
I tried applying this locally and there were a number of test failures. Could you please make sure to run check-llvm to run all the tests again and fix any failures. In particular, the following tests all failed for me, with what look to be issues related to your patch:
LLVM :: MC/ARM/dwarf-asm-multiple-sections-dwarf-2.s LLVM :: MC/ARM/dwarf-asm-multiple-sections.s LLVM :: MC/ARM/dwarf-asm-nonstandard-section.s LLVM :: MC/ARM/dwarf-asm-single-section.s LLVM :: MC/COFF/cfi-sections.s LLVM :: Object/objdump-relocations.test
@liadz0rz, I've checked and this change passes the tests locally, but you didn't answer this comment:
What email address and author values do you want me to include for your git commit message?
Please let me know, so that I can commit this for you.
Hey,
For the email please user liad.mordekoviz@gmail.com and for the author please use "Liad Mordekoviz",
Again thank you very much for all the kind help!
This broke tests everywhere when the experimental AVR backend is enabled, e.g. http://45.33.8.238/
Please take a look, and if it takes a while to fix please revert while you investigate.
I have something else going on on my machine at the moment, so this will be tricky for me to sort out. If somebody else could fix it, that would be great. The actual fix should be trivial. In llvm/test/MC/AVR/symbol_relocation.s, add the line ; CHECK-NEXT: OFFSET TYPE VALUE immediately after the CHECK for "RELOCATION RECORDS".
I finished what I was doing and submitted a quick fix. I haven't got the time to enable an experimental target on my machine, so the fix is technically speculative, but I'm confident in it. If there are more failures of the same kind, please consider fixing them directly rather than asking me to fix them.
Is it actually the llvm policy that build breakages/failures for experimental backends means reverting culprit patches? I thought that one of the things "experimental" means is that the build bot breakages specific to those backends are addressed by the owners of said experimental backend.
Oh, I missed that James already posted to llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2020-February/139115.html. Please reply there instead.
As a slight readability suggestion, consider doing the following:
; CHECK:RELOCATION RECORDS FOR [CODE]:
This allows for the ':' characters and therefore the actual checked-for text to line up.
Same applies a few lines below.