[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:773
+  SHeader.sh_addralign = YAMLSec ? (uint64_t)YAMLSec->AddressAlign : 1;
+  SHeader.sh_offset = alignToOffset(CBA, SHeader.sh_addralign, /*Offset=*/None);
+
----------------
FTR: You should probably use `YAMLSec->Offset` here instad of "None" to allow setting any offset using the "Offset" section key. (And this needs a test case). I think for this patch you can leave it as is though.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:787
+  else
+    llvm_unreachable("initDWARFSectionHeader() failed");
+
----------------
jhenderson wrote:
> Would this be better if it were more explicit? Something like: "initDWARFSectionHeader() should only be called if something describes the section".
It should not be `llvm_unreachable`, because it is possible to reach it using the following input:

```
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
Sections:
  - Name:    .debug_str
    Type:    SHT_DYNAMIC
    Flags:   [SHF_MERGE, SHF_STRINGS]
    Content: "610062006300"
DWARF:
```

(I guess it could be a error instead for start).


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:803
+  assignSectionAddress(SHeader, YAMLSec);
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80203