[PATCH] D92545: [llvm-readobj/elf] - Refine the implementation of "printFunctionStackSize".
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 7 01:51:04 PST 2020
grimar added inline comments.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5810-5811
const Elf_Shdr &StackSizeSec, DataExtractor Data, uint64_t *Offset) {
// This function ignores potentially erroneous input, unless it is directly
// related to stack size reporting.
+ uint32_t FuncSymIndex = 0;
----------------
jhenderson wrote:
> You've added several new warnings with this refactor. Does this comment still apply?
Probably we can remove it.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5841-5846
+ std::string Name = this->dumper().getStaticSymbolName(Index);
+ // Note: it is impossible to trigger this error currently, it is
+ // untested.
+ reportUniqueWarning("unable to get section of symbol '" + Name +
+ "': " + toString(SecOrErr.takeError()));
+ break;
----------------
jhenderson wrote:
> Should this not just be `cantFail`?
I am not sure.
`cantFail` looks like we give a guarantee for this condition to be always true. But this is not our intention.
This error currently can't be triggered because `Obj.getSection` API
is called somewhere inside `ElfObj.toSymbolRef(SymTab, Index).getAddress()`. But we don't give a guarantee here as
things are probably too brittle, unobvious and might change easily.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5874
if (*Offset == PrevOffset) {
- reportWarning(createStringError(object_error::parse_failed,
- "could not extract a valid stack size in " +
- describe(Obj, StackSizeSec)),
- FileName);
+ reportUniqueWarning("could not extract a valid stack size in " +
+ describe(Obj, StackSizeSec));
----------------
jhenderson wrote:
> Are you planning on further improving these messages in another change? The offset would be useful here, for example.
>
> (Suggested fix could be deferred too, if it causes too many test changes)
> Are you planning on further improving these messages in another change? The offset would be useful here, for example.
I'll do it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92545/new/
https://reviews.llvm.org/D92545
More information about the llvm-commits
mailing list