The main purpose of this patch is to add support for GOT section for MIPS target. Only reserved and global entries are supported for now.
In addition, the platform specific symbol "_gp" is added, which point to the start of .got section + 0x7ff0.
Details
Diff Detail
Event Timeline
By the way, what is your plan on MIPS support in ELF2? Is this patch just an experiment or you are going to port all MIPS related functionality from ELF to ELF2? As to me I plan to do that. But if you like and have a time to do that by yourself I will reschedule my work for Imagination accordingly.
ELF/Target.h | ||
---|---|---|
35 | I do not think the term FirstElement is a good choice. First - MIPS GOT contains two first elements so singular is not applicable. In fact "Module pointer" might be missed and all MIPS ABI consider these items as almost regular entries not as some sort of solid header. Second - both elements always have the same size as other GOT entries so it is enough to have a number of "first" elements. In that case we will not have to override this value for MIPS64. I would add GotHeaderEntriesNumber class field and rewrite the GotSection<ELFT>::addEntry: template <class ELFT> void GotSection<ELFT>::addEntry(SymbolBody *Sym) { Sym->GotIndex = Entries.size() + GotHeaderEntriesNumber; Entries.push_back(Sym); } |
My first goal was to finish with the GNU hash section and sorting requirements for dynamic symbol table. Because MIPS has its own requirements for the order of dynamic symbols, it looks as a good place to put my effort. At least, I plan to add sorting function and mandatory dynamic table entries.
Right now I have some time which I can spend to port MIPS functionality from ELF to ELF2. If you don't mind, we can synchronize our plans.
I'm going to post an updated version on Monday. I'll really appreciate any comments.
I have two more (almost done) patches, based on this one. I'm planning to post them on Monday, so the more nits I correct here, the less mess will be there.
- SymbolBody::GotIndex -> GlobalGotIndex
- TargetInfo::GotFirstElement -> GotHeaderEntries
ELF/Target.h | ||
---|---|---|
35 | Since we treat global symbols in a different way than local ones, I think we won't be able to set the GotIndex properly when we add support for local entries of GOT. My suggestion is to just rename the field GotIndex to GlobalGotIndex and use it accordingly, see the updated patch. |
ELF/Symbols.h | ||
---|---|---|
94 | This change does not look mandatory right now while you do not introduce local entries. Will you maintain local entries counter or first global entry index? Moreover how will it work for targets where GOT is solid and is not split into local/global parts? I suggest to keep this part of code unchanged and review it later with next patches. | |
ELF/Target.cpp | ||
684 | Do we really need both these separate constant? I think it is enough to keep them in the writeGotHeaderEntries. | |
691 | Are we sure that P[0] is initialized by zero? |
ELF/Target.cpp | ||
---|---|---|
684 | In this case we have to write it like (uintX_t(1) << (sizeof(uintX_t) * 8 - 1)) to be ready for 64bit targets. Is it correct? | |
691 | As far as I can understand, it's common practice here. See, for example, SymbolTableSection<ELFT>::writeTo(), which just skips the first entry rather than zero it. |
ELF/Target.cpp | ||
---|---|---|
684 | Well, I've thought about that variant. It seems a bit untidy for me because its rvalue is of type uint64_t and has to be truncated on 32bit platform. Sometimes it can produce warnings, so I prefer to avoid it. |
- Removed MipsTargetInfo<ELF32LE>::ModulePointer.
- Changed the calculation of SymbolBody::GotIndex.
- The test was broken.
Please note, this version has a little shortcoming. When creates DSO, it produces .rel.dyn section filled with R_MIPS_NONE records. I'm going to fix that in the following update(s).
ELF/SymbolTable.cpp | ||
---|---|---|
76–77 | resolve(new (Alloc) DefinedAbsolute.. | |
ELF/SymbolTable.h | ||
53–55 | This is inconsistent with other functions that add extra symbols. Other functions takes names and values. Can you change the signature of this function to void addAbsoluteSym(StringRef Name, uintX_t Value); | |
ELF/Target.cpp | ||
133 | In general please do not use mutable fields unless for cache or something. | |
691 | All writeTo() assumes that the buffer Buf points to is initialized with zero since that's a mmap'ed file that is newly created. | |
693 | Drop ull suffix. | |
730 | Since this function is used only once, remove that and inline here. write32<IsLE>(Loc, (read32<IsLE>(Loc) & 0xffff0000) | V); | |
749 | This line needs a comment for the magic number. | |
ELF/Writer.cpp | ||
137 | We do not try to push everything that is specific to each target to Target. For example, we have PPC64-specific code in the writer, and that made the entire code more readable. addSpecificSymbols and updateSpecificSymbols abstracted a concept of "target specific symbols", but currently it's used only for MIPS. I don't like that abstraction. I'd replace Target->updateSpecificSymbols() with to make it less abstract. if (Config->EMachine == EM_MIPS) updateGpSymbol(); |
- Move the functionality of adding and updating target specific symbols from TargetInfo to Driver and Writer.
- The DefinedAbsoluteSynthetic class is added to represent the synthetic absolute symbol which address can be updated.
- Address other review comments.
ELF/Driver.cpp | ||
---|---|---|
254 | Please describe what _gp symbols is in MIPS briefly instead of this generic comment. | |
ELF/OutputSections.cpp | ||
655 | I don't think we need a new type. I expected that we could create an Elf_Sym in the writer, sets Value to its st_value, and pass that object to DefinedAbsolute. | |
ELF/SymbolTable.cpp | ||
76 | Like this? typedef typename ELFFile<ELFT>::Elf_Sym Elf_Sym; auto *S= new (Alloc) Elf_Sym; memset(S, 0, sizeof(Elf_Sym)); S->st_value = Value; resolve(new (Alloc) DefinedAbsolute<ELFT>(Name, *S)); | |
ELF/Writer.cpp | ||
772 | It's better to mention that this is to backfill _gp symbol value for MIPS. |
ELF/OutputSections.cpp | ||
---|---|---|
655 | We have to reserve the place for the _gp symbol in the symbols and dynamic symbols sections. It requires the symbol to have all properties (like type, binding, etc.) to be set to the correct values because they can affect the sorting order. Therefore we can't use weak or undefined symbols for reserving the place. We have to update the value of the _gp symbol after assigning addresses for all the sections. We can't do it through adding a new Absolute symbol to the symbol table because it will produce the "duplicate symbol" error. Thus, we have to update the st_value field of the existent symbol. The DefinedAbsolute class has only the constant reference to the contained symbol, so it is required to use const_cast to update the st_value field. I'm not sure that it is a straightforward way to do the things, because it involves much indirect information that this particular symbol was intentionaly added (in Driver), it has reserved memory and not referenced to memmaped file (in SymTab), it was not replaced by another symbol (because of SymTab rules) and so on. The DefinedAbsoluteSynthetic class makes things more strict because it stores Elf_Sym within the class and gives explicit access to update the st_value field. |
I applied your patch locally to see what's the simplest way to do for _gp symbol. Can you please take a look at this? http://reviews.llvm.org/D14347 This is pretty similar to your second patch minus Target abstraction. This seems to be easiest. What do you think?
So, addAbosluteSym() eventually receives a reference to Elf_Sym. The question is, where to store Elf_Sym itself. I'd prefere to put it as a static member of the DefinedAbsolute class rather than the Out class, like we do for DefinedAbsolute::IgnoreUndef, Undefined::Required, etc.
- Remove DefinedAbsoluteSynthetic class, apply other changes from Rui's patch (the main difference is the place to store MipsGp variable).
- Fix the problem with filling GOT and relocations for DSO.
- Add the SHF_MIPS_GPREL flag for GOT section.
- The getMipsGpAddr() function now checks if the GOT section has VA set.
- Update the test.
- Add a link to the documentation
I'll commit it tomorrow if no more comments are added.
Please describe what _gp symbols is in MIPS briefly instead of this generic comment.