[ELF] Add .eh_frame pieces to map file

[andrewng]

Summary

Show the mapping of input .eh_frame pieces to the output .eh_frame section in the map file.

[ruiu]

Yeah, please add a test for this for demonstration purpose. Actually, for demonstration purpose, a test is probably more useful than the actual code, because we are more interested in knowing about how a program works as a result of a change than the change itself. In particular, I'm interested in knowing the information that is contained in the new output.

[jhenderson]

Thanks for the comments. This is currently on our backburner, as we've got some other work to do first, but hopefully @andrewng or I will be able to look at this at some point in the next couple of weeks. If I get a chance later today, I'll upload a test.

[andrewng]

Rebased and updated with minor change to make the output offset hex rather than decimal. Modified existing map file test to demonstrate the proposed .eh_frame output.

[ruiu]

Feature-wise this is OK but the code doesn't seem to fit well to other code in lld. Please read other part of the code and make your new code similar to that. In particular, please

• make functions shorter
• don't use auto and
• simplify code and logic.

[andrewng]

Updated to address comments.

Moved output of input section offset to toString as suggested by Rafael.

[ruiu]

ELF/InputSection.h

347 ↗

(On Diff #137220)

I don't think this is a good idea. lld::toString(T) is an overloaded function dispatched by its parameter type. But you are now defining a different function which you only use in MapFile.cpp. I'd move this back to the original file.

ELF/MapFile.cpp

141

This is still too long. When you add a large amount of code to an existing function, please try to make it simple -- by splitting it up, refactor, etc.

[ruiu]

ELF/MapFile.cpp

143

As I said, please try to make your new code look the same as the existing code. For example, we don't define multiple local variables in one line like this in other places.

[andrewng]

Updated to address review comments.

However, happy to abandon in favour of D44168 if that is the preferred implementation and output format.

[ruiu]

Yeah, I think I slightly prefer my implementation because even though it is slightly inefficient (because it creates an intermediate vector), it is a bit easier to read. Or you can swap the function body with mine if you want.

[andrewng]

There are also some slight differences in the output between this implementation and the one in D44168. This patch does not output the "+0x0" for a zero offset and outputs the input section's alignment for the "Align" column. Which style of output is preferred?

[• rafael]

Rui Ueyama via Phabricator via llvm-commits
<[e-mail]> writes:

ruiu added inline comments.

Comment at: ELF/InputSection.h:347

-std::string toString(const elf::InputSectionBase *);
+std::string toString(const elf::InputSectionBase *, uint64_t SecOffset = 0);

} // namespace lld

I don't think this is a good idea. lld::toString(T) is an overloaded function dispatched by its parameter type. But you are now defining a different function which you only use in MapFile.cpp. I'd move this back to the original file.

My objection to the original implementation was that it was looking at
the output of toString, effectively knowing exactly what toString does
and breaking the modularity.

Would you be OK with having

std::string lld::someName(const InputSectionBase *Sec, uint64_t Offset);

and implementing std::string lld::toString(const InputSectionBase *Sec)
as someName(Sec, 0)?

Cheers,
Rafael

[ruiu]

I don't think omitting +0x<hex> only when it is +0x0 doesn't make much sense. It is better to always print out an offset regardless of its value. That also makes it easy to parse by machine.

Does alignment make sense? I think that each section has an alignment, but each piece in an .eh_frame doesn't have a notion of an alignment. So I think it isn't correct to show a section alignment as a piece alignment.

[andrewng]

Abandoning this revision in favour of D44168.

ELF/MapFile.cpp

void elf::writeMapFile() {
 
   // Print out file contents.
   for (OutputSection *OSec : OutputSections) {
     writeHeader(OS, OSec->Addr, OSec->Size, OSec->Alignment);
     OS << OSec->Name << '\n';
 
     // Dump symbols for each input section.
     for (InputSection *IS : getInputSections(OSec)) {
+      if (IS == InX::EhFrame) {
+        uint64_t InputStart = 0, InputEnd = 0, OutputStart = 0, OutputEnd = 0;
+
+        auto AddEntry = [&](InputSectionBase *IS) {
+          writeHeader(OS, (OSec->Addr + OutputStart), (OutputEnd - OutputStart),
+                      IS->Alignment);
+
+          std::string InputName = toString(IS);
+          if (InputStart) {
+            auto Pos = InputName.size();
+            if (Pos && (InputName.back() == ')'))
+              --Pos;
+            InputName.insert(Pos, "+0x" + utohexstr(InputStart, true));
+          }
+
+          OS << indent(1) << InputName << '\n';
+        };
+
+        InputSectionBase *IS = nullptr;
+
+        auto ProcessPiece = [&](const EhSectionPiece *P) {
+          uint64_t InputOff = P->InputOff;
+          uint64_t OutputOff = P->OutputOff;
+          if ((P->Sec != IS) || (OutputOff != OutputEnd) ||
+              (InputOff != InputEnd)) {
+            if (IS)
+              AddEntry(IS);
+            InputStart = InputOff;
+            OutputStart = OutputOff;
+            IS = P->Sec;
+          }
+
+          auto Size = P->Size;
+          OutputEnd = OutputOff + Size;
+          InputEnd = InputOff + Size;
+        };
+
+        auto CieRecords = InX::EhFrame->getCieRecords();
+        for (const auto *Rec : CieRecords) {
+          ProcessPiece(Rec->Cie);
+
+          const auto &Fdes = Rec->Fdes;
+          for (const auto *Fde : Fdes)
+            ProcessPiece(Fde);
+        }
+
+        if (IS)
+          AddEntry(IS);
+
+        continue;
+      }
+
       writeHeader(OS, OSec->Addr + IS->OutSecOff, IS->getSize(), IS->Alignment);
       OS << indent(1) << toString(IS) << '\n';
       for (Symbol *Sym : SectionSyms[IS])
         OS << SymStr[Sym] << '\n';
     }
   }
 }

ELF/SyntheticSections.h

public:
   size_t NumFdes = 0;
 
   struct FdeData {
     uint32_t Pc;
     uint32_t FdeVA;
   };
 
   std::vector<FdeData> getFdeData() const;
+  std::vector<CieRecord *> const &getCieRecords() const { return CieRecords; }
 
 private:
   uint64_t Size = 0;
 
   template <class ELFT, class RelTy>
   void addSectionAux(EhInputSection *S, llvm::ArrayRef<RelTy> Rels);
 
   template <class ELFT, class RelTy>

test/ELF/Inputs/map-file2.s

 foo:
+.cfi_startproc
 nop
+.cfi_endproc
 .global bar
 bar:
 nop
 .section .text.zed,"ax",@progbits
 .global zed
 zed:

test/ELF/map-file.s

 // RUN: llvm-ar rc %t4.a %t4.o
 // RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so -o %t -M | FileCheck -strict-whitespace %s
 // RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so -o %t -print-map | FileCheck -strict-whitespace %s
 // RUN: ld.lld %t1.o %t2.o %t3.o %t4.a %t5.so -o %t -Map=%t.map
 // RUN: FileCheck -strict-whitespace %s < %t.map
 
 .global _start
 _start:
+.cfi_startproc
+.cfi_endproc
  .quad sharedFoo
  .quad sharedBar
  .byte 0xe8
  .long sharedFunc1 - .
  .byte 0xe8
  .long sharedFunc2 - .
  .byte 0xe8
  .long baz - .
 .global _Z1fi
 _Z1fi:
 .cfi_startproc
-.cfi_endproc
 nop
+.cfi_endproc
 .weak bar
 bar:
 .long bar - .
 .long zed - .
 local:
 .comm   common,4,16
 .global abs
 abs = 0xAB5
 labs = 0x1AB5
 
 // CHECK:      Address          Size             Align Out     In      Symbol
 // CHECK-NEXT: 00000000002001c8 0000000000000078     8 .dynsym
 // CHECK-NEXT: 00000000002001c8 0000000000000078     8         <internal>:(.dynsym)
 // CHECK-NEXT: 0000000000200240 000000000000002c     8 .gnu.hash
 // CHECK-NEXT: 0000000000200240 000000000000002c     8         <internal>:(.gnu.hash)
 // CHECK-NEXT: 000000000020026c 0000000000000030     4 .hash
 // CHECK-NEXT: 000000000020026c 0000000000000030     4         <internal>:(.hash)
 // CHECK-NEXT: 000000000020029c 0000000000000031     1 .dynstr
 // CHECK-NEXT: 000000000020029c 0000000000000031     1         <internal>:(.dynstr)
 // CHECK-NEXT: 00000000002002d0 0000000000000030     8 .rela.dyn
 // CHECK-NEXT: 00000000002002d0 0000000000000030     8         <internal>:(.rela.dyn)
 // CHECK-NEXT: 0000000000200300 0000000000000030     8 .rela.plt
 // CHECK-NEXT: 0000000000200300 0000000000000030     8         <internal>:(.rela.plt)
-// CHECK-NEXT: 0000000000200330 0000000000000030     8 .eh_frame
-// CHECK-NEXT: 0000000000200330 0000000000000030     8         <internal>:(.eh_frame)
+// CHECK-NEXT: 0000000000200330 0000000000000060     8 .eh_frame
+// CHECK-NEXT: 0000000000200330 000000000000002c     8         {{.*}}{{/|\\}}map-file.s.tmp1.o:(.eh_frame)
+// CHECK-NEXT: 0000000000200360 0000000000000014     8         {{.*}}{{/|\\}}map-file.s.tmp1.o:(.eh_frame+0x2c)
+// CHECK-NEXT: 0000000000200378 0000000000000018     8         {{.*}}{{/|\\}}map-file.s.tmp2.o:(.eh_frame+0x18)
 // CHECK-NEXT: 0000000000201000 000000000000002d     4 .text
 // CHECK-NEXT: 0000000000201000 0000000000000028     4         {{.*}}{{/|\\}}map-file.s.tmp1.o:(.text)
 // CHECK-NEXT: 0000000000201000 0000000000000000     0                 _start
 // CHECK-NEXT: 000000000020101f 0000000000000000     0                 f(int)
 // CHECK-NEXT: 0000000000201028 0000000000000000     0                 local
 // CHECK-NEXT: 0000000000201028 0000000000000002     4         {{.*}}{{/|\\}}map-file.s.tmp2.o:(.text)
 // CHECK-NEXT: 0000000000201028 0000000000000000     0                 foo
 // CHECK-NEXT: 0000000000201029 0000000000000000     0                 bar