Page MenuHomePhabricator

[llvm-objcopy] Add --localize-symbol option
ClosedPublic

Authored by paulsemel on Apr 25 2018, 8:13 AM.

Details

Summary

This option permit to localize a symbol by given its name.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Apr 25 2018, 8:13 AM

Looks mostly good to me, just need to merge the passes over the symbols.

tools/llvm-objcopy/llvm-objcopy.cpp
197–214

Accumulate the functions so that localize can happen in a single pass. See what I did above with sections. Also in https://reviews.llvm.org/D46029 I requested that localize be replaced with a slightly more generalized 'map over symbols'. You can go ahead and write this in terms of localize but depending on which patch goes though first one or the other of you will need to change some code. Just a heads up.

jhenderson added inline comments.Apr 26 2018, 2:42 AM
test/tools/llvm-objcopy/localize.test
2

Why do we need all the extra symbols in this test?

Could we also get tests for localizing a symbol that is already a local, and also localizing a weak symbol, please, and for localizing multiple symbols at the same time (maybe these could be all one test).

I think we've also tended to test both short and long options.

tools/llvm-objcopy/Opts.td
61

Let's use a slightly more meaningful help message, that doesn't just repeat the switch name, e.g. "Mark <symbol> as local" (see also --localize-hidden).

paulsemel marked 3 inline comments as done.Apr 26 2018, 7:30 AM
paulsemel added inline comments.
tools/llvm-objcopy/llvm-objcopy.cpp
197–214

Alright, I'll keep this in mind, thanks ! :)

paulsemel updated this revision to Diff 144116.Apr 26 2018, 7:32 AM
paulsemel marked an inline comment as done.

Added Jake and James suggestions :

  • improved help message
  • removed multiple calls to localize function
  • improved tests

Aside from the conflict with D46029, mentioned already, and a couple of minor test-related points, this looks fine from my point of view.

test/tools/llvm-objcopy/localize.test
32

Since the visibility isn't relevant to the test, you can get rid of this. I'd also rename all the symbols to be simpler named (i.e. something like "Local", "Weak" and "Global"). The default/hidden part was to do with the symbol visibility from the localize-hidden test (I assume?), to reflect which was which.

paulsemel updated this revision to Diff 144123.Apr 26 2018, 8:00 AM
paulsemel marked an inline comment as done.

Added James' suggestions :

  • renamed symbol names in tests
  • removed unnecessary visibility field in tests
This revision is now accepted and ready to land.Apr 26 2018, 10:26 AM

LGTM

Should I wait for https://reviews.llvm.org/D46029 ? Or do you think I can commit ?

You can land it whenever suits you. Whoever lands it first makes the other person refactor their code a touch but it isn't a big deal. I'll alert the other person of the conflict when it happens.

You can land it whenever suits you. Whoever lands it first makes the other person refactor their code a touch but it isn't a big deal. I'll alert the other person of the conflict when it happens.

Ok, I am going to land it, and if Alexander needs help to rebase, I will help him (it might be really easy actually)

paulsemel closed this revision.Apr 26 2018, 10:49 AM

Commited on r330963