[PATCH] Add support for subsections to the ELF assembler. Fixes PR8717.

Peter Collingbourne peter at pcc.me.uk
Tue Apr 9 23:47:53 PDT 2013


  > Please add a test showing this.

  To be clear, I'll add a test making sure we don't emit a relocation in this case.

  > Please make sure we reject numbers larger than 8192.

  I thought about this, but a seemingly undocumented feature of gas is that it supports larger numbers, as well as negative numbers, so I decided to omit the check (although it's debatable whether we need to be compatible on this).  If we decide not to support this we can probably simplify getSubsectionInsertionPoint as well.


================
Comment at: include/llvm/MC/MCObjectStreamer.h:63
@@ +62,3 @@
+    CurSectionData->getFragmentList().insert(CurInsertionPoint, F);
+    F->setParent(CurSectionData);
+  }
----------------
Rafael Ávila de Espíndola wrote:
> Currently setParent is unused. The default sectiondata values are also unused. Instead of creating dangling fragments that have to immediately be passed to insert, could you change the MCFragment constructor to take an insertion point?
> 
It seems better to me to be able to deal with the insertion logic in this function rather than having to repetitively deal with insertion throughout the MCObjectStreamers.  In my opinion, repetition would make it easier to make a mistake in one of the streamers and (e.g.) have fragments appended to the fragment list rather than inserted in the right place.

================
Comment at: lib/MC/MCObjectStreamer.cpp:179
@@ +178,3 @@
+      !Subsection->EvaluateAsAbsolute(IntSubsection, getAssembler()))
+    report_fatal_error("Cannot evaluate subsection number");
+  CurInsertionPoint =
----------------
Rafael Ávila de Espíndola wrote:
> Can we report the error without using report_fatal_error?
Yes, but this would be a relatively invasive QOI improvement to handle a relatively rare occurrence; it would probably entail changing ChangeSection and SwitchSection to return an error value, and SwitchSection has a large number of callers that would each need to be audited to make sure they're handling errors correctly.

================
Comment at: include/llvm/MC/MCAssembler.h:596
@@ +595,3 @@
+  /// below that number.
+  std::map<int64_t, MCFragment *> SubsectionFragmentMap;
+
----------------
Rafael Ávila de Espíndola wrote:
> Subsections are a number from 0 to 8192, there has to be a better map for this :-)
> 
> Given that most files will not have many subsections, a sorted SmallVector will probably do.
Agree.


http://llvm-reviews.chandlerc.com/D598



More information about the llvm-commits mailing list