[PATCH] D92545: [llvm-readobj/elf] - Refine the implementation of "printFunctionStackSize".

[George Rimar via Phabricator via llvm-commits]

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