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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:803
+  assignSectionAddress(SHeader, YAMLSec);
+}
+
----------------
grimar wrote:
> I'd suggest the following code change:
> 
> ```
> static bool shouldEmitDWARF(Optional<DWARFYAML::Data> DWARF,
>                             StringRef SecName) {
>   return StringSwitch<bool>(SecName)
>       .Case(".debug_str", !DWARF->DebugStrings.empty())
>       .Default(false);
> }
> 
> template <class ELFT>
> static uint64_t emitDWARF(typename ELFT::Shdr &SHeader, StringRef Name,
>                           const DWARFYAML::Data &DWARF, raw_ostream &OS) {
>   uint64_t BeginOffset = OS.tell();
>   if (Name == ".debug_str")
>     DWARFYAML::EmitDebugStr(OS, DWARF);
>   else
>     llvm_unreachable("unexpected emitDWARF() call");
>   return OS.tell() - BeginOffset;
> }
> 
> template <class ELFT>
> void ELFState<ELFT>::initDWARFSectionHeader(Elf_Shdr &SHeader, StringRef Name,
>                                             ContiguousBlobAccumulator &CBA,
>                                             ELFYAML::Section *YAMLSec) {
>   zero(SHeader);
>   SHeader.sh_name = DotShStrtab.getOffset(ELFYAML::dropUniqueSuffix(Name));
>   SHeader.sh_type = YAMLSec ? YAMLSec->Type : ELF::SHT_PROGBITS;
>   SHeader.sh_addralign = YAMLSec ? (uint64_t)YAMLSec->AddressAlign : 1;
>   SHeader.sh_offset = alignToOffset(CBA, SHeader.sh_addralign, /*Offset=*/None);
> 
>   ELFYAML::RawContentSection *RawSec =
>       dyn_cast_or_null<ELFYAML::RawContentSection>(YAMLSec);
>   if (Doc.DWARF && shouldEmitDWARF(*Doc.DWARF, Name)) {
>     if (RawSec && (RawSec->Content || RawSec->Size))
>       reportError("cannot specify the '" + Name +
>                   "' section contents in the 'DWARF' entry and the 'Content' "
>                   "or 'Size' in the 'Sections' entry at the same time");
>     else
>       SHeader.sh_size = emitDWARF<ELFT>(SHeader, Name, *Doc.DWARF, CBA.getOS());
>   } else if (RawSec) {
>     SHeader.sh_size = writeContent(CBA.getOS(), RawSec->Content, RawSec->Size);
>   } else {
>     reportError("not supported"); // ??
>   }
> 
> ....
> ```
> 
> Benefits are:
> 1) `sh_size` is set in a single place.
> 2) "cannot specify the ...Content/SIze ..." error is reported before emitting the DWARF data and not after.
> 3)  no need to have `generateContentsFromDWARFEntry` member.
BTW, you even can avoid having the 

```
static bool shouldEmitDWARF(Optional<DWARFYAML::Data> DWARF,
                            StringRef SecName) {
  return StringSwitch<bool>(SecName)
      .Case(".debug_str", !DWARF->DebugStrings.empty())
      .Default(false);
}
```

Because `getUsedSectionNames()` provides you all information needed
to avoid checking `!DWARF->DebugStrings.empty()` here again (it seems).
Perhaps ``getUsedSectionNames()` could return `MapVector<StringRef>` and you could
do something like:

```
static bool shouldEmitDWARF(Optional<DWARFYAML::Data> DWARF,
                            StringRef SecName) {
  MapVector<StringRef> M = DWARF.getUsedSectionNames();
  if (!SecName.empty() && SecName[0] == '.' && M.count(SecName.drop_front())
    return true;
  return false;
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203