[PATCH] D131290: [llvm-objdump] Create name for fake sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:656
+  if (!Index) {
+    // no section string table.
+    if (!FakeSectionStrings.empty())
----------------
MaskRay wrote:
> `// There is no section name string table. Return FakeSectionStrings which is non-empty if we have created fake sections.`
@namhyung, I believe @MaskRay was suggesting using the comment he's posted instead of "no section string table", as an improvement. Please update.


================
Comment at: llvm/include/llvm/Object/ELF.h:784
   for (auto Phdr : *PhdrsOrErr) {
-    if (!(Phdr.p_type & ELF::PT_LOAD) || !(Phdr.p_flags & ELF::PF_X))
+    if ((Phdr.p_type != ELF::PT_LOAD) || !(Phdr.p_flags & ELF::PF_X)) {
+      ++Idx;
----------------
namhyung wrote:
> MaskRay wrote:
> > namhyung wrote:
> > > I've found a bug in this code - it should check equality.
> > The convention does not place parens around `!=`
> Yeah, but I thought it'd good to be symmetric to the RHS.
Having the "symmetry" has the potential to actually cause confusion, since the LHS isn't negated unlike the RHS.


================
Comment at: llvm/test/Object/objdump-no-sectionheaders.test:2
+## This test checks llvm-objdump -h can handle ELF files without section info.
+## Only PT_LOAD program with PF_X flag will be display corresponding fake sections.
 
----------------



================
Comment at: llvm/test/Object/objdump-no-sectionheaders.test:22
+    NoHeaders:       true
+ProgramHeaders:
+  - Type:            PT_PHDR
----------------
I think I see a potential problem here, which could explain the problem you're seeing: you haven't specified any contents or explicit offsets of any of these program headers, which may mean that even though they have addresses specified, they'll start at the same offset within the file. This might result in something bad such as only the one section appearing in the output, although I don't know the llvm-objdump code well enough to be certain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131290