[PATCH] D65313: [llvm-readelf] Dump the stack sizes sections with --stack-sizes

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

The first comments are below.



================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:30
+    Type:    SHT_PROGBITS
+    Content: "000000000000000010000000000000000020"
+    Link:    .text
----------------
This needs a comment I think, i.e. what does this content describe.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:40
+    Relocations:
+# A symbol relative reference. 
+      - Offset: 0
----------------
`##`


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:113
+## of the .stack_sizes section.
+## 
+# RUN: yaml2obj --docnum=3 %s > %t03
----------------
You do not need the last line.
We usually either separate the comment from `RUN:` with just an empry line  (though sometimes not, just be consistent).


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:163
+  Type: ET_REL
+  Machine: EM_X86_64
+Sections:
----------------
Please align like you did above.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:186
+    Section: .text
+    Type:    STT_SECTION
+  - Name:    foo
----------------
Do you need section symbol?


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:286
+  - Name:    foo
+# An invalid section index.
+    Index:   10
----------------
`##`


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:295
+# RUN: yaml2obj --docnum=8 %s > %t08
+# RUN: llvm-readelf --stack-sizes %t08 2> %t08.err | FileCheck %s --check-prefix=NORELOCSECTION-OUT
+# RUN: FileCheck %s < %t08.err --check-prefix=NORELOCSECTION-ERR -DFILE=%t08
----------------
We usually use 2>&1. I am not sure it worth to chech the output separatelly here.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:323
+# RUN: llvm-readelf --stack-sizes %t09 %t10 | FileCheck %s --check-prefix=MULTIPLE
+
+## Check that we handle an archive.
----------------
I'd expect to see "#      MULTIPLE:" lines right here. You can probably test `ARCHIVE` just separatelly.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:359
+# RUN: llvm-readelf --all %t14 2> %t14.err | FileCheck %s --check-prefix=NONFUNCTIONSYM
+# RUN: FileCheck %s < %t14.err --check-prefix=NONFUNCTIONSYM-ERR -DFILE=%t14
+
----------------
Again, probably I'd not test errors separatelly. (i.e. would use 2>&1)


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:392
+
+# RUN: yaml2obj --docnum=10 %s > %t15
+# RUN: not llvm-readelf --stack-sizes %t15 2>&1 | FileCheck %s --check-prefix=UNSUPPRELOC -DFILE=%t15
----------------
Add a description?


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:395
+
+# UNSUPPRELOC: error: '[[FILE]]': unsupported relocation type in section .rela.stack_sizes
+
----------------
Perhaps you should print the relocation name too.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4315
+      // issues directly related to stack size dumping.
+      consumeError(SymAddrOrErr.takeError());
+      continue;
----------------
This looks worrying. Why do you need this?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4334
+    if (FuncNameOrErr)
+      FuncName = maybeDemangle(*FuncNameOrErr);
+    else
----------------
This probably need a test (I think I didn't see one for testing demangled names)


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4336
+    else
+      consumeError(FuncNameOrErr.takeError());
+  } else
----------------
Why?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4378
+    else
+      consumeError(NameOrErr.takeError());
+
----------------
Why?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4398
+    else
+      consumeError(RelocSymValueOrErr.takeError());
+  }
----------------
Why?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4416
+template <class ELFT>
+void GNUStyle<ELFT>::printStackSizes(const ELFObjectFile<ELFT> *Obj) {
+  bool HeaderHasBeenPrinted = false;
----------------
This method is very long. If there is any way to make it shorter, that would be great.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4433
+    return SectionRef(DRI, Obj);
+  };
+
----------------
Seems you can use `ELFObjectFile::toDRI()` to hide work with `DRI.p` and move this out to make it static helper probably?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4486
+  // their corresponding relocation sections.
+  using SectionMap = llvm::MapVector<SectionRef, SectionRef>;
+  SectionMap StackSizeRelocMap;
----------------
Its a bit strange to see `using` here. You use `SectionMap` only once
and it is not common do do in the missle of function I think.
I'd just remove.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4550
+
+    bool (*Supports)(uint64_t);
+    RelocationResolver Resolver;
----------------
`Supports` probably not a very good name for function. May be `IsSupportedFn`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5436
+  W.printString(
+      "Dumping of stack sizes in LLVM style is not implemented yet\n");
+}
----------------
I think it worth adding the `llvm-readobj` calls to your test case too. So that it will be visible what result we have and what is the place to change when we implement this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65313/new/

https://reviews.llvm.org/D65313