Hi Akira,
This is looking good. Some specific comments on the details below.
Thanks!
Jim
diff --git a/include/llvm/MC/MCELFObjectWriter.h b/include/llvm/MC/MCELFObjectWriter.h
index 6e9f5d8..220ecd0 100644
--- a/include/llvm/MC/MCELFObjectWriter.h
+++ b/include/llvm/MC/MCELFObjectWriter.h
@@ -13,6 +13,7 @@
#include "llvm/MC/MCObjectWriter.h"
#include "llvm/Support/DataTypes.h"
#include "llvm/Support/ELF.h"
+#include <vector>
namespace llvm {
class MCELFObjectTargetWriter {
@@ -27,6 +28,33 @@ protected:
uint16_t EMachine_, bool HasRelocationAddend_);
public:
+ /// @name Relocation Data
+ /// @{
+
+ struct ELFRelocationEntry {
+ // Make these big enough for both 32-bit and 64-bit
+ uint64_t r_offset;
+ int Index;
+ unsigned Type;
+ const MCSymbol *Symbol;
+ uint64_t r_addend;
+ const MCFixup *fixup;
+
+ ELFRelocationEntry()
+ : r_offset(0), Index(0), Type(0), Symbol(0), r_addend(0), fixup(0) {}
+
+ ELFRelocationEntry(uint64_t RelocOffset, int Idx,
+ unsigned RelType, const MCSymbol *Sym,
+ uint64_t Addend, const MCFixup *Fixup)
+ : r_offset(RelocOffset), Index(Idx), Type(RelType),
+ Symbol(Sym), r_addend(Addend), fixup(Fixup) {}
+
+ // Support lexicographic sorting.
+ bool operator<(const ELFRelocationEntry &RE) const {
+ return RE.r_offset < r_offset;
+ }
+ };
+
I don't think this really belongs to the MCELFObjectTargetWriter class, per se. I suggest moving it outside of the class definition.
static uint8_t getOSABI(Triple::OSType OSType) {
switch (OSType) {
case Triple::FreeBSD:
@@ -52,6 +80,8 @@ public:
virtual void adjustFixupOffset(const MCFixup &Fixup,
uint64_t &RelocOffset);
+ virtual void ReorderRelocs(const MCAssembler &Asm,
s/ReorderRelocs/reorderRelocs/. Function names start w/ a lower case letter. Personally, I prefer naming the prefix "sort" rather than "reorder", as it's a bit more descriptive, but not a big deal either way.
+ std::vector<ELFRelocationEntry>& Relocs);
The '&' binds to the identifier, not the type name, and should be formatted as such. I.e., space before the '&' and no space between it and "Relocs".
/// @name Accessors
/// @{
diff --git a/lib/MC/ELFObjectWriter.cpp b/lib/MC/ELFObjectWriter.cpp
index 36f94b4..093eb07 100644
--- a/lib/MC/ELFObjectWriter.cpp
+++ b/lib/MC/ELFObjectWriter.cpp
@@ -84,31 +84,7 @@ class ELFObjectWriter : public MCObjectWriter {
}
};
- /// @name Relocation Data
- /// @{
-
- struct ELFRelocationEntry {
- // Make these big enough for both 32-bit and 64-bit
- uint64_t r_offset;
- int Index;
- unsigned Type;
- const MCSymbol *Symbol;
- uint64_t r_addend;
-
- ELFRelocationEntry()
- : r_offset(0), Index(0), Type(0), Symbol(0), r_addend(0) {}
-
- ELFRelocationEntry(uint64_t RelocOffset, int Idx,
- unsigned RelType, const MCSymbol *Sym,
- uint64_t Addend)
- : r_offset(RelocOffset), Index(Idx), Type(RelType),
- Symbol(Sym), r_addend(Addend) {}
-
- // Support lexicographic sorting.
- bool operator<(const ELFRelocationEntry &RE) const {
- return RE.r_offset < r_offset;
- }
- };
+ typedef MCELFObjectTargetWriter::ELFRelocationEntry ELFRelocationEntry;
Scoping operators shouldn't be typedefed away. Spell it out explicitly when the type is referenced. It makes the code clearer, though a bit more verbose. That said, with the above tweak to move the relocation type out to the top level, there shouldn't need to be any explicit scope resolution.
/// The target specific ELF writer instance.
llvm::OwningPtr<MCELFObjectTargetWriter> TargetObjectWriter;
@@ -786,7 +762,7 @@ void ELFObjectWriter::RecordRelocation(const MCAssembler &Asm,
else
assert(isInt<32>(Addend));
- ELFRelocationEntry ERE(RelocOffset, Index, Type, RelocSymbol, Addend);
+ ELFRelocationEntry ERE(RelocOffset, Index, Type, RelocSymbol, Addend, &Fixup);
Relocations[Fragment->getParent()].push_back(ERE);
}
@@ -1072,8 +1048,7 @@ void ELFObjectWriter::WriteRelocationsFragment(const MCAssembler &Asm,
MCDataFragment *F,
const MCSectionData *SD) {
std::vector<ELFRelocationEntry> &Relocs = Relocations[SD];
- // sort by the r_offset just like gnu as does
- array_pod_sort(Relocs.begin(), Relocs.end());
+ TargetObjectWriter->ReorderRelocs(Asm, Relocs);
Please add a comment explaining a bit. Nothing elaborate, just something along the lines of, "Sort the relocation entries. Most targets just sort by r_offset, but some (e.g., MIPS) have additional constraints."
for (unsigned i = 0, e = Relocs.size(); i != e; ++i) {
ELFRelocationEntry entry = Relocs[e - i - 1];
diff --git a/lib/MC/MCELFObjectTargetWriter.cpp b/lib/MC/MCELFObjectTargetWriter.cpp
index 15bf476..4f3e3b2 100644
--- a/lib/MC/MCELFObjectTargetWriter.cpp
+++ b/lib/MC/MCELFObjectTargetWriter.cpp
@@ -7,6 +7,7 @@
//
//===----------------------------------------------------------------------===//
+#include "llvm/ADT/STLExtras.h"
Since we're moving the sort here from ELFObjectWriter.cpp, it may be possible to remove the STLExtras.h include from the latter. Please check and see.
#include "llvm/MC/MCELFObjectWriter.h"
using namespace llvm;
@@ -36,3 +37,10 @@ const MCSymbol *MCELFObjectTargetWriter::ExplicitRelSym(const MCAssembler &Asm,
void MCELFObjectTargetWriter::adjustFixupOffset(const MCFixup &Fixup,
uint64_t &RelocOffset) {
}
+
+void
+MCELFObjectTargetWriter::ReorderRelocs(const MCAssembler &Asm,
+ std::vector<ELFRelocationEntry>& Relocs) {
'&' binding thing again.
+ //
Not original with you, but since we're in here anyway, this should be a well-formed sentence: "Sort by the r_offset, just like gnu as does."
+ array_pod_sort(Relocs.begin(), Relocs.end());
Trailing whitespace.