Currently, if the note name is known, but the value isn't we don't print
the contents.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Since this extends llvm-readelf output, can you also send an email to binutils@sourceware.org and check what output they want for GNU readelf? For llvm-readobj output, we are not bound to GNU decisions. Though, if the GNU output is not bad, aiming for consistency can usually make our implementation simpler.
Is there a test case that shows we don't print the raw data when the decoding is known?
Not yet, but I've added a test case that would previously not print the contents for a GNU note of an unknown type.
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
---|---|---|
4893 | It seems like GNU readelf does not print the contents in this case. |
I assume that you've verified against GNU readelf all the behaviour that you're changing here?
llvm/test/tools/llvm-readobj/ELF/gnu-note-size.test | ||
---|---|---|
2 | It's unclear to me what this test is testing, so I can't really say whether this change is good. Would you mind adding a comment at the top of the test, either in this patch or a different one, briefly explaining its purpose, please? I imagine you might need to dig into the old commit messages for this file to find out. | |
llvm/tools/llvm-readobj/ELFDumper.cpp | ||
4893 | Nit: missing trailing full stop. |
llvm/test/tools/llvm-readobj/ELF/gnu-note-size.test | ||
---|---|---|
5–6 | ## for comments. | |
llvm/test/tools/llvm-readobj/ELF/note-amd.s | ||
29 | Maybe .note.unknown rather than invalid? Invalid to me suggests a recognised type, but broken data. | |
llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s | ||
90 | Unless you're planning on fixing this immediately, it would be worth mentioning this test in the bug, or the TODO might get missed. | |
llvm/test/tools/llvm-readobj/ELF/note-amdgpu.test | ||
106 | As above: maybe .note.unknown for clarity? | |
llvm/test/tools/llvm-readobj/ELF/note-generic.s | ||
4 ↗ | (On Diff #321370) | This looks wrong? Also, I'm not sure why this test is changing as part of this change? |
Possibly this is a patch ordering thing, but some of the pre-merge bots are failing for this patch, I think. LGTM, apart than that.
It's unclear to me what this test is testing, so I can't really say whether this change is good. Would you mind adding a comment at the top of the test, either in this patch or a different one, briefly explaining its purpose, please? I imagine you might need to dig into the old commit messages for this file to find out.