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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
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:
> > 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.
> > Why allow .text.foo, but not .stack_sizes.foo for example?
> I had a time to think about this a bit. `text.foo` does not have a Link or Info fields set. It has to have a
> function name encoded in the section name. On the other side `.stack_sizes` have `Link`.
> My vision is that руку it is a dumping tools problem probably: readelf has a format that makes hard to show a useful output here,
> though it has full information. I.e. it shows Link as a numeric value what makes matching task for a human harder.
> We can change/improve it in llvm-readobj. I am not sure it worth to add section suffixes to a binary produced because of that.
> Our aim is not to make human readable objects, we have set of tools for dumping them. We might want to improve them.
> 
> 
"is that руку it is a ..." -> "is that here it is a ..."


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

https://reviews.llvm.org/D67757