This is an archive of the discontinued LLVM Phabricator instance.

[Object/ELF] Add support for setVisibility()
ClosedPublic

Authored by davide on Mar 11 2015, 6:16 PM.

Details

Summary

Simon introduced getVisibility() a while ago but now we need setVisibility() in order to implement visibility for ELF symbols in lld.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 21794.Mar 11 2015, 6:16 PM
davide retitled this revision from to [Object/ELF] Add support for setVisibility().
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: atanasyan, lhames, Bigcheese.
davide set the repository for this revision to rL LLVM.
davide added a project: lld.
davide added subscribers: Unknown Object (MLST), emaste.
atanasyan added inline comments.Mar 11 2015, 10:38 PM
include/llvm/Object/ELFTypes.h
199 ↗(On Diff #21794)

The st_other field might contain other flags except STV_xxx. If one sets such flags and then call setVisibility its overwrites the st_other field completely. Maybe write this function something like this:

STV_MASK = 3;

void setVisibility(unsigned char v) {
    assert(v < 4 && "Invalid value for visibility");
    st_other = (st_other & ~STV_MASK) | v;
}
davide updated this revision to Diff 21799.Mar 11 2015, 11:32 PM
davide removed rL LLVM as the repository for this revision.

You're right, my bad. I updated the patch. When I was looking at this file I noticed that there's a fair amount of magic hardcoded in there. I left SVT_MASK spelled at 0x3 and I'll plan to audit it entirely and change all the hardcoded values with properly defined constants. Does this make sense to you?

atanasyan edited edge metadata.Mar 12 2015, 12:13 AM

That sounds like a good plan.

This revision was automatically updated to reflect the committed changes.