[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

140

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

142

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

OS << left_justify("Address", W) << ' ' << left_justify("Size", W)
      << " Align Out     In      Symbol\n";
 
   // 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 (BaseCommand *Base : OSec->SectionCommands) {
       auto *ISD = dyn_cast<InputSectionDescription>(Base);
       if (!ISD)
         continue;
       for (InputSection *IS : ISD->Sections) {
+        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, "+" + std::to_string(InputStart));
+            }
+
+            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>