- User Since
- May 16 2017, 11:34 AM (56 w, 6 d)
Mon, Jun 11
I'll have more comments tomorrow. I'm still grokking my way though llvm-readobj as I've never looked at that code before.
Thu, Jun 7
Ah yes. Thanks!
Wed, Jun 6
Hey I'm away but I got a bug report requesting -x as an alias in
llvm-strip. I haven't looked at the code for this patch so it might already
be solved here but I'd appreciate it being added. No worries either way.
I'll just add it when aight get back of it isn't there.
Wed, May 30
Quick drop in. What I was thinking about before was to just never pass the
null symbol to remove or anything else. It can still be there it just
shouldn't be allowed to be updated or removed.
Fri, May 25
This is causing breaks in fuchsia,
Thu, May 24
nit: Add comment above loop but otherwise LGTM. Feel free to submit after that.
As always the right solution winds up 10000 times simpler than anything I considered in the interim. This looks good to me after fixing Alex's and my comments.
Landed in rL333136
Wed, May 23
Switched to use index. Fixed to print out full buffer in case no newline exists.
Tue, May 22
Modified to group multiple lines into a single flush but still flush at least once if a newline is found.
Yeah I agree with Alex and James here. I think I'd prefer some sort of interface extension. Something as simple as 'markSymbols' that's done as a loop though the sections which then just sets a Boolean like you had originally. That should a) give the desired time complexity and b) not be super complicated. That loop can also only be triggered if --strip-uneeded is used which is nice.
Just a heads up I'm not convinced my shared_ptr idea is ok. I want to
consider what Alex and James are saying. My hope has always been to keep
the symbol table as the owner so if Alex has a proposal I want to read it
Mon, May 21
I didn't think about that. I think the key thing here then is to use shared_ptr to store all symbols and then use use_count to keep track of the references.
LGTM but please wait on james
LGTM with a couple additions.
Yeah I haven't heard back from Lexan nor have I tried reproducing such a build myself and if we're that close to not relaying on this sort of hack I'd much rather wait on this than risk breaking someone for the sake of a stopgap for such a short period of time. @echristo Can you link the review to that?
One little thing and this LGTM
May 18 2018
May 17 2018
I'll be back to respond further. I'm pressed for time today.b
May 16 2018
May 15 2018
I forgot to tag this. Closing.
May 14 2018
I'm generally ok with this but I think some important checks need to occur first. Namely I think I'd like to check that Chromium still builds with this option since I'd imagine they'd be affected by this.
If it isn't too much trouble I'd like to wait until --strip-unneeded is implemented.
May 11 2018
Ping. Do you need anything else from one of us?
May 9 2018
Alex did this (and this is a stupidly old change)
Alex did this
May 8 2018
Spoke offline, LGTM
This LGTM with a couple fixes but please wait on James for the final approval.
Need some clarification on this option
LGTM: Please wait on James as I'd like his input on how we should test this sort of stuff going forward as well.
May 7 2018
LGTM I checked that this is consistent with objcopy (and it would seem other tools as well). Seems acceptable to me as is.
May 4 2018
May 3 2018
LGTM but please wait on James
Can you see what GNU objcopy does when the symbol is defined (both local and global cases)? Roland mentioned that in the defined case it might actually transform the relocation into a section relative relocation instead! That'd be pretty neat IMO but is likely out of the scope of this diff (even with added complexity already mentioned). If it turns out that GNU objcopy emits a section reletive relocation in that case then we can add a TODO there.
I totally forgot about those isues! Yeah we don't want to leave dangling references around. I really should have caught that issue but I was thinking. I just figured I had already worked out all the issues with symbol removal when I wrote removeSectionReferences but you don't have to worry about those sections sticking around in that case where you *do* have to worry about it in the generic symbol removal case. The behavior I'd like to see here is that the relocations referencing a remove symbol cause an error to be thrown. I'm considering what I want to happen for groups but I belive it should also cause an error if you try to remove the symbol of a group section. I couldn't find any other Symbol pointers and that is the canonical way to refer to a pointer so I believe James caught all the cases. This is going to require extending the hierarchy with a special 'removeSymbolReferences' method in SectionBase and then adding a special 'removeSymbols' function in Object...sorry this just got a lot more complicated.
May 2 2018
May 1 2018
I'm happy with this change. I think we should change --globalize and --localize to match but if --globalize-symbol or --localize-symbol is used it shouldn't bother checking; if people want to do non-sense that's their choice. I didn't think about it but it makes a lot of sense not to globalize weak symbols and it makes sense to not localize undefined symbols. So I think the choices objcopy chose makes sense. If you could change those to match that, it would be awesome!
Apr 30 2018
So I asked the gods about this and got a half response on this issue but this appears to be intended for localize and globalize and *might* have been intended for weaken. After some thought I don't think it makes a lot of sense to weaken an undefined symbol and it might be unexpected behavior so I'm cool with it working this way. That would be my preference and it gives compatibility with objcopy.
Apr 27 2018
Apr 26 2018
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.
Apr 25 2018
Looks mostly good to me, just need to merge the passes over the symbols.
Apr 24 2018
Apr 11 2018
ran format with style=llvm (git-clang-format seems to not have a way to do this?) and fixed a couple other nits
Apr 10 2018
Updated to (in the future) match Alex's plan for how we can expand this.
Apr 9 2018
Apr 5 2018
This looks good to me. Thanks!
Apr 3 2018
I split the "Config" up into "Config" and "CopyAction". The plan is for "CopyAction" to handle multiple formats and for "Config" to determine which sort of CopyAction is constructed. When COFF support rolls around we should be able to use CopyAction for those as well. Archives support is also something I plan on adding eventually and that will be a copy action composed of many copy actions.
Mar 26 2018
Mar 12 2018
LGTM as well