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

Mon Sep 23 08:14:09 PDT 2019

jhenderson added inline comments.

================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:183
+  static bool nameMatches(StringRef Name) {
+    return Name.startswith(".stack_sizes");
+  }
----------------
grimar wrote:
> 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
I know this is due to how LLD and other linkers behave, but I still find it less than helpful that you can't have sections with different suffixes for many section types. Why allow .text.foo, but not .stack_sizes.foo for example? I'd personally prefer to change LLD and MC (but that's another change) than change llvm-readelf. That way, it's immediately recognisable which section a .stack_sizes section is associated with when looking at the output, without needing to decipher the Link field.

================
Comment at: lib/ObjectYAML/ELFYAML.cpp:1179
+  if (const auto *SS = dyn_cast<ELFYAML::StackSizesSection>(Section.get())) {
+    if (SS->Content && SS->Entries)
+      return ".stack_sizes can't have both Content and Entries tags";
----------------
If we're allowing people to specify a Content field, it seems reasonable to also allow specification of a Size field, with the content set to zeroes, if not otherwise filled in by Content.

================
Comment at: test/tools/llvm-readobj/stack-sizes.test:508
+    Type:    SHT_PROGBITS
+    Content: ""
+    Link:    .text
----------------
I'd either put this at the bottom of the tag and not change the indentation of the other fields, or indent the fields in the other sections to match. That being said, it seems a bit weird to do it this way. I'd think adding one or more explicit entries would be a more natural choice (or at least an empty Entries array).

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

================
Comment at: test/tools/obj2yaml/elf-stack-sizes.yaml:3
+
+## Check that obj2yaml uses `Entries` tag to describe .stack_sizes section
+## when it can extract <address, size> pairs.
----------------
uses the "Entries" tag to describe a .stack_sizes section

================
Comment at: test/tools/obj2yaml/elf-stack-sizes.yaml:35
+
+## Check that obj2yaml uses `Content` tag to describe .stack_sizes section
+## when can't extract the entries. For example, when section data is truncated.
----------------
uses the "Content" tag to describe a .stack_sizes section when it can't extract the entries, for example, when ...

================
Comment at: test/tools/obj2yaml/elf-stack-sizes.yaml:63
+
+## Check obj2yaml can dump the empty .stack_sizes.
+
----------------
Delete "the"

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:1
+## Check how yaml2obj produces .stack_sizes sections.
+
----------------
I think you need Big Endian and 32-bit test cases here, since those affect how the section is written.

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:3
+
+## The first test:
+## 1) Shows we can produce a .stack_sizes section from the description with
----------------
Maybe change this to "Test the following cases when the .stack_sizes Content field is specified:"

"The first test:" doesn't seem useful to me.

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:4
+## The first test:
+## 1) Shows we can produce a .stack_sizes section from the description with
+##    a valid section content.
----------------
from the -> from a

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:6
+##    a valid section content.
+## 2) Shows we can produce a incorrect .stack_sizes section from the description with
+##    a broken (truncated) section content.
----------------
a incorrect -> an incorrect
from the -> from a

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:8
+##    a broken (truncated) section content.
+## 3) Shows we can produce an empty .stack_sizes section from the description with
+##    an empty section content.
----------------
from the -> from a
with an empty -> with empty

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:14
+
+## Case 1: a valid content is used.
+# CHECK:      Section {
----------------
I'd simplify these to "Case 1: valid content." "Case 2: truncated content." and "Case 3: empty content."

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:90
+
+## Check we can omit `Address` tag. In this case address part will be zeroed.
+
----------------
omit the "Address" tag.
case the address will be zero.

================
Comment at: test/tools/yaml2obj/elf-stack-sizes.yaml:151
+
+## Check we must specify either `Content` or `Entries` tag when describe .stack_sizes.
+
----------------
describe -> describing

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

https://reviews.llvm.org/D67757