[llvm-readobj][ELF] Prints hex format values in lower-case mode #156683
Conversation
Currently llvm-readobj prints e_entry address in uppercase mode, however all other places such as symbols, relocs the addresses are printed in lowercase mode. This PR aims to print the addresses in uniform manner. Other use-case: FileCheck comparison is case-sensitive by default, so if you would like to compare e_entry address with the same address in the symbol table it won't match by default.
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-llvm-binary-utilities Author: Ádám Kallai (kaadam) ChangesCurrently llvm-readobj prints e_entry address in uppercase mode, however all other places such as 'symbols', 'relocs', the addresses are printed in lowercase mode. This PR aims to print the addresses in uniform manner. Other use-case: Full diff: https://github.com/llvm/llvm-project/pull/156683.diff 1 Files Affected:
diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp
index a440bad130f4c..79a6ce1fd88b8 100644
--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -3640,7 +3640,7 @@ template <class ELFT> void GNUELFDumper<ELFT>::printFileHeaders() {
printFields(OS, "Machine:", Str);
Str = "0x" + utohexstr(e.e_version);
printFields(OS, "Version:", Str);
- Str = "0x" + utohexstr(e.e_entry);
+ Str = "0x" + utohexstr(e.e_entry, /*LowerCase=*/true);
printFields(OS, "Entry point address:", Str);
Str = to_string(e.e_phoff) + " (bytes into file)";
printFields(OS, "Start of program headers:", Str);
|
There was a problem hiding this comment.
Thanks for the PR! In principle, I think this makes sense. The majority of our hex printing is via Twine::utohexstr, which is always lower-case. I think the missing lower case argument in the calls to llvm::utohexstr are likely just oversights and should be updated.
There are several other places in this file that use llvm::utohexstr, some of which don't use the lower-case argument. Please could you update them all (and any tests that fail as a result of course). I note that in some cases (e.g. e_version), it's unlikely to matter due to the range of values that could be printed, but it's better to be consistent.
Adjusted the related tests to the newer format.
|
@jh7370 Thanks for your review. I've updated the |
There was a problem hiding this comment.
llvm::utohexstr defaults to upper-case while Twine::utohexstr defaults to lower-case. I think lower-case is more popular and llvm-readelf always uses lower-case.
Using more lower-case for llvm-readobj makes sense. Unfortunately, changing some fields (relocation offsets and addends) for llvm-readobj might cause a lot of changes.
|
llvm/docs/ReleaseNotes.md
Outdated
| * `llvm-readelf` now dumps all hex format addresses in lower-case mode generally. | ||
| Therefore the corresponding tests have been udpated as well. |
There was a problem hiding this comment.
No need to mention the tests in the release notes.
Also, this isn't specific to addresses any longer (e.g. you've made changes that impact printing of flags, version numbers and so on), so please drop references to them from this release note and the PR title and description. Something generic like "hex format values" would be enough.
There was a problem hiding this comment.
I'd suggest some slight grammar changes to the PR description:
Previously, llvm-readelf dumped hex format values in different ways. Some of them were printed in upper-case, while the others were in lower-case format. This change switches the format to lower-case in all cases.
Why is this useful? As an example, FileCheck comparisons are case-sensitive by default. This change means it's easier to compare those values, because they have the same format.
I've dropped the "where it is possible" bit. There aren't any within the llvm-readelf code that aren't possible, right?
You can also drop the "the corresponding tests have been updated as well" since that's normal procedure.
With those tweaks and the one nit in the release note, LGTM. Do you need assistance merging, or do you have commit access already?
llvm/docs/ReleaseNotes.md
Outdated
| @@ -151,6 +151,8 @@ Changes to the Debug Info | |||
| Changes to the LLVM tools | |||
| --------------------------------- | |||
|
|
|||
| * `llvm-readelf` now dumps all hex format values in lower-case mode generally. | |||
|
@jh7370 Thank you for your thorough review and the suggestions. Updated the description. I have no commit access yet. May I ask you to merge it? |
|
Done, please keep a close eye out for build bot failure notifications. |
Previously, llvm-readelf dumped hex format values in different ways. Some of them were printed in upper-case, while the others were in lower-case format. This change switches the format to lower-case in all cases.
Why is this useful? As an example, FileCheck comparisons are case-sensitive by default. This change means it's easier to compare those values, because they have the same format.