Page MenuHomePhabricator

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

Authored by paulsemel on Apr 27 2018, 1:01 AM.

Details

Summary

This option globalize the given symbol

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Apr 27 2018, 1:01 AM
jhenderson added inline comments.Apr 27 2018, 1:10 AM
test/tools/llvm-objcopy/globalize.test
19–24

You might as well get rid of this section, since you have no symbols in it.

tools/llvm-objcopy/llvm-objcopy.cpp
327–330

Two points regarding this and the SymbolsToLocalize bit above, which I only just noticed:

  1. The .empty() query is unnecessary, since std::begin(<empty vector>) == std::end(<empty vector>), so find in this situation will always return as not found.
  2. Could we factor out the std::find != std::end common code between this and the localize variation into something like a lambda or helper called contains/inVector or similar? (It might be worth checking that something like that doesn't already exists).
paulsemel updated this revision to Diff 144301.Apr 27 2018, 2:22 AM
paulsemel marked 2 inline comments as done.

Applied James' suggestions :

  • Refactor the std::find code into a contains function
  • Removed useless section in test
  • Removed useless check if (!Config.SymbolsToXX.empty())
alexshap added inline comments.Apr 27 2018, 8:27 AM
tools/llvm-objcopy/llvm-objcopy.cpp
324

empty() was not completely useless here imo - this flag is probably not the most frequently used flag, thus when it's not specified the "early check" would save us at least 3 function calls per symbol (at least in the debug builds) (std::begin/end/find)

tools/llvm-objcopy/llvm-objcopy.h
38 ↗(On Diff #144301)

if it's only used in llvm-objcopy.cpp - maybe move it there ? (and make it static to avoid putting it into the global namespace)

alexshap removed 1 blocking reviewer(s): alexshap.Apr 27 2018, 8:28 AM
alexshap removed 1 blocking reviewer(s): alexshap.Apr 27 2018, 8:29 AM
alexshap added inline comments.Apr 27 2018, 8:39 AM
tools/llvm-objcopy/llvm-objcopy.h
39 ↗(On Diff #144301)

it seems to me, that the name Vector is kind of misleading here - this function (as is) is more generic - maybe replace Vector with Sequence or Container ? + change Elt to smth like X (but that's just an opinion, not super important)

paulsemel updated this revision to Diff 144356.Apr 27 2018, 8:56 AM
paulsemel marked 3 inline comments as done.

Added Alexander suggestions

tools/llvm-objcopy/llvm-objcopy.cpp
324

Yes, tbh I think you're right on this one, it might somehow save us time !

tools/llvm-objcopy/llvm-objcopy.h
39 ↗(On Diff #144301)

Agree with you, I misnamed because I was probably thinking to the use I will make of this function !

alexshap accepted this revision.Apr 27 2018, 9:13 AM

added one more inline comment, otherwise, to me this looks good, but i would wait for Jake and James

tools/llvm-objcopy/llvm-objcopy.cpp
138

maybe i'm missing smth - but why not simply return bool ?

This revision is now accepted and ready to land.Apr 27 2018, 9:13 AM

Sure, I will wait for them to review too ! Thanks for reviewing btw :)

tools/llvm-objcopy/llvm-objcopy.cpp
138

This line is here to ensure we are passing the function a container. Otherwise, it throws an error at CT.
This is a trick I've seen once, but maybe it's not a good practice ?

alexshap requested changes to this revision.Apr 27 2018, 9:20 AM
alexshap added inline comments.
tools/llvm-objcopy/llvm-objcopy.cpp
139

well, actually, even better - let's delete this and make use of
./STLExtras.h:967 bool is_contained(R &&Range, const E &Element)

This revision now requires changes to proceed.Apr 27 2018, 9:20 AM
paulsemel added inline comments.Apr 27 2018, 9:25 AM
tools/llvm-objcopy/llvm-objcopy.cpp
139

Oups.. I'm sorry, I completely missed this function..
Changing to this one ! Thanks !

paulsemel updated this revision to Diff 144360.Apr 27 2018, 9:34 AM
paulsemel marked an inline comment as done.

Removed contains implementation and used the is_contained.

alexshap accepted this revision.Apr 27 2018, 9:35 AM
This revision is now accepted and ready to land.Apr 27 2018, 9:35 AM
paulsemel closed this revision.Apr 27 2018, 12:13 PM

Committed on r331068

jhenderson added inline comments.Apr 30 2018, 1:46 AM
tools/llvm-objcopy/llvm-objcopy.cpp
324

I think this is a case of premature optimization. A standard library implementation of .empty() might easily just use the distance between the begin and end iterator, so might end up with just as many (potentially more) calls as saved. Similarly, an optimizer might be able to optimize the calls.

In other words: unless performance measurements show differently, I'd prefer to keep the code simpler.