Page MenuHomePhabricator

[ELF][MIPS] Delete GotSection::addMipsLocalEntry method
ClosedPublic

Authored by atanasyan on Mar 20 2016, 5:03 AM.

Details

Summary

Now local symbols have SymbolBody so we can handle all kind of symbols in the GotSection::addEntry method. The patch moves the code from addMipsLocalEntry to addEntry.

Diff Detail

Event Timeline

atanasyan updated this revision to Diff 51127.Mar 20 2016, 5:03 AM
atanasyan retitled this revision from to [ELF][MIPS] Delete GotSection::addMipsLocalEntry method.
atanasyan updated this object.
atanasyan added reviewers: ruiu, rafael.
atanasyan set the repository for this revision to rL LLVM.
atanasyan added a project: lld.
atanasyan added a subscriber: llvm-commits.
ruiu edited edge metadata.Mar 20 2016, 8:08 AM

I'm not very sure whether this is positive for readability or not. This seems to be neutral to me. If you could just remove the code that would be fine, but this patch add a new if (Body.isPreemptible()), which is a bit difficult to understand. Is there any way to simplify it further?

Note that I'm not against this patch.

In D18302#378884, @ruiu wrote:

I'm not very sure whether this is positive for readability or not. This seems to be neutral to me. If you could just remove the code that would be fine, but this patch add a new if (Body.isPreemptible()), which is a bit difficult to understand. Is there any way to simplify it further?

Unfortunately the if (Body.isPreemptible()) is mandatory because we do not need to add to dynamic symbol table symbols which have entries in the "local" part of MIPS GOT. I can add a comment to describe this if statement. Even after that the code is shorter.

rafael edited edge metadata.Mar 21 2016, 7:50 AM

Probably OK. Can you just fix these comments and upload a patch for another look?

ELF/OutputSections.cpp
93 ↗(On Diff #51195)

A local symbol is not preemptable, so you can simplify the if.

ELF/Writer.cpp
408 ↗(On Diff #51195)

Can this now be moved to addEntry? It is getting a "if mips" in this patch.

atanasyan updated this revision to Diff 51195.Mar 21 2016, 11:26 AM
atanasyan edited edge metadata.
  • Simplified condition in the GotSection::addEntry method.
  • Move the Sym.MustBeInDynSym = true to the GotSection::addEntry method.
ruiu accepted this revision.Mar 21 2016, 11:43 AM
ruiu edited edge metadata.

I will try to find a way to handle this MIPS ABI oddity in some more readable way, but this is OK for now (and I'm not sure if such better way exists). LGTM.

By the way, I'm on vacation this week, so my response would be a bit slow this week.

ELF/OutputSections.cpp
93 ↗(On Diff #51195)

I think we need a brief explanation what we are doing here for MIPS even if it is a duplicate with a comment in other place.

This revision is now accepted and ready to land.Mar 21 2016, 11:43 AM
rafael accepted this revision.Mar 21 2016, 11:57 AM
rafael edited edge metadata.

LGTM too.

This revision was automatically updated to reflect the committed changes.

Thanks for review.