[PATCH] D80203: [ObjectYAML][DWARF] Add DWARF entry in ELFYAML.

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked an inline comment as done.
Higuoxing added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1254
+    // For sections identified by their names, we have to set the section type
+    // for them earlier.
+    if (ELFYAML::StackSizesSection::nameMatches(Name))
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I'd add the following sentence to this comment: "This means they can have other SHT_* type values, but that the special parsing of those types is not supported."
> > > 
> > > This seems like a reasonable solution to me, but I'm interested to here @grimar's thoughts.
> > If I understand it right, that change means:
> > 1) We are unable to create, for example, `SHT_DYNAMIC` section named  `.debug_` anymore.
> > 2) It was done to allow creating `.debug_` sections that has `sh_type` overriden to something, like to `SHT_DYNAMIC`.
> > 
> > Currently, yaml2obj uses section type to make a specific section mapping. It looks generally more right than checking sections by name (in the ideal world). Unfortunately we have to check section names still. But it is just because they have no dedicated SHT_* types first of all I believe.
> > 
> > And my concern that (2) might be confusing, because the current way we have to override things is to use Sh* fields. E.g:
> > 
> > ```
> >   // This can be used to override the offset stored in the sh_name field.
> >   // It does not affect the name stored in the string table.
> >   Optional<llvm::yaml::Hex64> ShName;
> > 
> >   // This can be used to override the sh_offset field. It does not place the
> >   // section data at the offset specified.
> >   Optional<llvm::yaml::Hex64> ShOffset;
> > 
> >   // This can be used to override the sh_size field. It does not affect the
> >   // content written.
> >   Optional<llvm::yaml::Hex64> ShSize;
> > 
> >   // This can be used to override the sh_flags field.
> >   Optional<llvm::yaml::Hex64> ShFlags;
> > ```
> > 
> > Also, what if we want to create a `SHT_RELR` section with the type `SHT_DYNAMIC`?
> > 
> > I think the better way would be to introduce `ShType` field that would allow to override the `sh_type` to anything.
> > (And I believe it can be done in a follow-up patch, no need to support such specific things from the begining).
> > 
> > How does it sound?
> > 
> To clarify.
> 
> This will create a dynamic section with the name `.debug_str`.
> ```
> - Name: .debug_str
>   Type: SHT_DYNAMIC
> ```
> 
> And this then can be used to create a debug section with the type `SHT_DYNAMIC`:
> 
> ```
> - Name: .debug_str
>   Type: SHT_PROGBITS
>   ShType: SHT_DYNAMIC
> ```
Thank you @grimar, It sounds reasonable to me since we've already had `shName/shSize...` fields to override section header. Besides, it's more flexaible to create sections with uncommon properties. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203