I think it should be possible if you make the relocation point to the object itself (which will have local linkage) and not the alias that you're creating.

Could this use a relative relocation so that we aren't paying the cost of a dynamic relocation for each global?

@pcc Sorry I missed your comment

I'm still confused. If the behavior intended only has to do with the existence of relocations why are we making this change? A clear difference of behavior needs to be pointed out and that hasn't been made clear.

@paulsemel Any comments?

Any comments on this?

I'm still a bit unclear. Does it strip symbols that are duplicates of symbols in .dynsym? Does it just get rid of the the whole symbol table no matter what?

If we always align first PT_TLS section on 64 bytes wouldn't this fix the problem?

I'm confused as to what this is trying to accomplish.

I see you originally added it in https://reviews.llvm.org/rL353919.

Does this match GNU objcopy's behaviour? If GNU strips them, we should probably too. If not, we shouldn't.

We decide what to internalize after computeDeadSymbols, so why wouldn't we be able to use this to block internalization of linkonce_odr global variables (when !ReadOnly)?

I agree this doesn't look like the correct fix, especially the check !Sym.isExecutable() which doesn't look correct to me

Looks like I have another idea how this can be fixed. In addition to 'ReadOnly' we can introduce 'CompileTimeConstant' flag in GlobalVarSummary.
Not marking linkonce_odr and weak_odr data symbols which are preserved and are not 'CompileTimeConstant' live will fix PR41645.
Also such flag allows exploring some new optimization opportunities:

Addressed comments

Ok, as far as I understood from ongoing discussion, we can first implement --special-syms as a dummy flag. That said, I'm updating the diff according to comments from @jhenderson

Splitted IHEX patch into reader and writer. Diff now contains the "writer" part.

Ping

It isn't working, because llvm::computeDeadSymbols keeps non-prevailing linkonce_odr and weak_odr symbols live.
It is possible to check for function summary when doing this to fix the issue:

if (S->linkage() == GlobalValue::AvailableExternallyLinkage ||
    ((S->linkage() == GlobalValue::WeakODRLinkage ||
    S->linkage() == GlobalValue::LinkOnceODRLinkage) && isa<FunctionSummary>(S->getBaseObject())))
  KeepAliveLinkage = true;

however this will prevent optimizing out linkonce_odr and weak_odr constants

LGTM, thanks. I suggest waiting for 1-2 days in case @tejohnson has something to say.

There's a lot of code to review here.

I've responded to some of the comments, meanwhile I'm splitting patch into writer (will go first) and reader (will go next). Will update the review soon

Rebased

Ok, I finally have some time to work on this again
Changes

How about making a symlink nm -> llvm_nm and use different behavior depending on tool name?
The same approach is used by llvm-addr2line, AFAIR.

what's wrong with keeping it like it is?

So what are further actions with this? Is emulating GNU nm seems like a better choice?

Can't operand 0 of bitcast (%union.A* %u) be used as a key?

"MutatedMemory" is only used to hold evaluated store values

I wonder if it's possible to stop using MutatedMemory for checks in ComputeLoadResult and instead start using separate map which can also be filled
in EvaluateBlock. In this new map you can use bitcast (or even first operand of bitcast) as a key.

btw, some people at euro llvm also requested srec supprt, which seems extremely similar to ihex

My only concern with this patch: how long does this test take to run, specifically, how long does llvm-mc take to generate a file with this many sections?

Addressed comments

Addressed

Addressed comments from @MaskRay

Addressed

Looks to me like you've hit a bug here

Okay, thanks. llvm-nm shows them by default, I take it?

FYI, most of the other llvm-objcopy developers are still away following Euro LLVM

Ping

Added test case, addressed comments.

That was helpful, thanks! I'll update diff shortly

Test case?

Line parser moved to IHexRecord::parse
Better test case for reader
Addressed some of review comments

Addressed comments

Addressed review comments

New implementation taken from libarchive (dynamic crc table)

It doesn't matter, at most a single memory fence is necessary.

Yep. However, you don't have it in your code. The volatile int crc_tbl_inited prevents compiler optimizations but doesn't prevent out-of-order execution as far as I know.

@aprantl

Shouldn't this be only conditionally compiled if zlib isn't available?

don't bother with the 1KB table in both binary and source, just recompute it on the first use

I suspect that this is going to interact nastily with D59843.

This should be rebased against r354850. Otherwise LGTM.