[PATCH] D67757: [yaml2obj/obj2yaml] - Add 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");
+  }
----------------
jhenderson wrote:
> 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.
The main concern is that .stack_sizes.foo can increase the object file size considerably.  I agree that .stack_sizes.foo is immediately recognisable but I am not sure such trade-off is worthwhile. .stack_sizes is not alone - some sanitizers synthesized sections (e.g. .sancov_cntrs) are not suffixed, either. Lots of .group are not suffixed, etc.


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

https://reviews.llvm.org/D67757