[PATCH] D67757: [yaml2obj/obj2yaml] - Add a support for .stack_sizes sections.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:183
+  static bool nameMatches(StringRef Name) {
+    return Name.startswith(".stack_sizes");
+  }
----------------
I think we can just use `Name == ".stack_sizes"` for now. MC doesn't create `.stack_sizes.*`

Moreover, the error messages below just use plain `.stack_sizes`.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:794
+    ContiguousBlobAccumulator &CBA) {
+  typedef typename ELFT::uint uintX_t;
+  raw_ostream &OS =
----------------
using


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:1155
+      if (ELFYAML::StackSizesSection::nameMatches(Name))
+        Section.reset(new ELFYAML::StackSizesSection());
+      else
----------------
`Section = std::make_unique<ELFYAML::StackSizesSection>();`


================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:1
+## Check how yaml2obj produce .stack_sizes sections from YAML descriptions.
+
----------------
produces

>  from YAML descriptions.

Seems redundant. This directory contains all such YAML files but none mentions "YAML descriptions".


================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:7
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --sections --section-data %t1 | FileCheck %s --check-prefix=CONTENT-VALID
+
----------------
Is it tighter to put CONTENT-VALID, CONTENT_INVALID and CONTENT-TRUNCATED tests together?

```
# Valid
- Name: .stack_sizes [1]
  Type:    SHT_PROGBITS
  Content: ...
# Truncated
- Name: .stack_sizes [2]
  Type:    SHT_PROGBITS
  Content: ...
# ...
```


================
Comment at: tools/obj2yaml/elf2yaml.cpp:293
+      // We still might have a special section, which has a regular type.
+      // Such sections are recognized by name.
+      Expected<ELFYAML::Section *> SpecialSecOrErr = dumpSpecialSection(&Sec);
----------------
Move the `if (Shdr->sh_type == ELF::SHT_PROGBITS)` check here. It responds to the claim "which has a regular type."

Then, we probably should reword the comment. One suggestion I get is "recognize some special SHT_PROGBITS sections by name."


================
Comment at: tools/obj2yaml/elf2yaml.cpp:494
+  ArrayRef<uint8_t> Content = *ContentOrErr;
+  if (!Content.empty()) {
+    DataExtractor Data(StringRef(reinterpret_cast<const char *>(Content.data()),
----------------
I guess the if can be deleted. An empty .stack_sizes can be processed with the same code.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:495
+  if (!Content.empty()) {
+    DataExtractor Data(StringRef(reinterpret_cast<const char *>(Content.data()),
+                                 Content.size()),
----------------
If D67797 is accepted, this code can switch to the new ctor.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:509
+      // If the data of .stack_sizes section is broken in any way,
+      // we dump it as array of bytes.
+      consumeError(Cur.takeError());
----------------
as an array of bytes.

> If the data of .stack_sizes section is broken in any way,

How about "if .stack_sizes cannot be decoded"?


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

https://reviews.llvm.org/D67757