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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:183
+  static bool nameMatches(StringRef Name) {
+    return Name.startswith(".stack_sizes");
+  }
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > MaskRay wrote:
> > > > 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`.
> > > I am not sure here. I did it because llvm-readobj test case uses YAML with `.stack_sizes.*`:
> > > https://github.com/llvm-mirror/llvm/blob/master/test/tools/llvm-readobj/stack-sizes.test#L35
> > > 
> > > I do not know was it intention or the person who wrote the test just did not know how to desribe sections with the same name in YAML.
> > > It probably might be not that bad to support `.stack_sizes.*` too (and may be teach MC to produve them)?
> > > Or we can fix `llvm-readobj` test first. Or go with `.stack_sizes.*` here and deside what to do in a follow-up patch for YAML+readobj.
> > > What would you prefer?
> > `Name.startswith(".stack_sizes");` is fine if we eventually emit `.stack_sizes.*` for clang -ffunction-sections -fstack-size-section. However, there are two problems:
> > 
> > * Larger code size. `.stack_sizes.foo`, `.text.foo` and `.rodata.foo` can take large amount of space in `.strtab`. This is why `-fno-unique-section-names` is sometimes useful.
> > * Linkers don't combine `.stack_sizes.*` into `.stack_sizes`. ld.bfd and lld will have to be taught the special rule. In lld, it is in `getOutputSectionName`.
> Seems the right way for now is to fix `llvm-readobj` then. I'll do a patch.
Patch for `llvm-readobj`: D67824


================
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
+
----------------
MaskRay wrote:
> 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: ...
> # ...
> ```
Ok. But I left all fields for the first case, because we probably need to test them all at least once:
since the section has a special handling, we need to verify which values we set by default.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:494
+  ArrayRef<uint8_t> Content = *ContentOrErr;
+  if (!Content.empty()) {
+    DataExtractor Data(StringRef(reinterpret_cast<const char *>(Content.data()),
----------------
MaskRay wrote:
> I guess the if can be deleted. An empty .stack_sizes can be processed with the same code.
Almost "yes" :) I had to modify the code slightly and added a test case for testing this case, which asserted before, but now works fine.
(Issue was that either 'Entries' or 'Content' must present to pass section validation, but in the previous version I did not set any of them when the section data was empty).

After removing 'if', without the code modificatiuon, yaml2obj for an empty section would produce `Entries: []` instead of `Content: ''`,
what perhaps is not critical, but seems do not match the comment saying that "If .stack_sizes cannot be decoded, we dump it as an array of bytes." (assuming that empty section == broken section == can not be decoded).


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


================
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());
----------------
MaskRay wrote:
> 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"?
> How about "if .stack_sizes cannot be decoded"?
LG.


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

https://reviews.llvm.org/D67757