assume that case is not "obvious" when offset is not zero.

BTW, does it make sense to make this patch work for static variables first?

I wonder if the same could be achieved with a clever use of ifunc?

Do you mean generating ifunc for each instrumented global and tagging a pointer there?

Addressed

Changed way X0 and X1 are assigned on AArch64

even for internal / non-preemptible globals.

Addressed review comments

Addressed comments

Addressed

Addressed

Addressed some of review comments
Unfortunately I don't know how to use memset from __hwasan_memset, given this comment in hwasan.cc

// ACHTUNG! No system header includes in this file.

Addressed review comments. Now the patch uses approach identical to ASAN.
ASAN doesn't add prefix in kernel compilation mode (my guess is that memset and friends are part of Linux kernel and are always instrumented in such scenario)

In fact, I changed my mind - let's do __hwasan_memset.

What about kernel compilation mode?

Added test case

Would it be more flexible to use the same approach here?

Could you tell me the commands I need to run to test this?

Hello David,

ASan uses -Wl,-whole-archive to pull all its members

don't think so. It's just symbol interposition. Every module in the process gets the same definition.

How do you do this now?

LGTM

LGTM

Unfortunately, this is not an option. I have very custom heap implementation (in fact multiple heap types sharing contiguous memory block), so ASAN heap can't be a drop-in replacement. I'm using manual poisoning and it works pretty well, but have to disable C++ runtime each time I build with ASAN.

Thanks!

Oh, my bad. opt -function-import gets index from llvm-lto2, not from opt -thinlto-bc. Sorry about that

Why won't it work for distributed builds? The read only gvar flag is being serialized out and back in, so shouldn't it show up in the distributed indexes?

Sorry I have to suggest small amendment to this patch before committing after looking test/ThinLTO/X86/distributed_import.ll.
It looks like distributed import is using per-module indexes, so internalization of RO vars will not work correctly. To overcome this
I suggest adding a check for propagateConstants to have been run before marking GV for internalization

• Addressed review comments
• Reverted changes in llvm-link.cpp
• No longer clearing read only attribute on dead variables in propagateConstants. As a result the following tests were updated

test/Linker/funcimport.ll
test/Transforms/FunctionImport/funcimport.ll

• Changed isLive() to isGlobalValueLive() in propagateConstants

Rebased and addressed review comments

the call to propagateConstantsToRefs only on function summaries

I'm not sure why we couldn't import and make a local copy of the aliasee when an alias to it is preserved and/or not eligible to import

Can you please post the *.index.bc file? It is saved before dot, so it should be there

• Addressed review comments
• Added call to propagateConstants to createCombinedModuleSummaryIndex to support -thinlto mode of llvm-lto

I see, yes that explanation makes sense. I suppose on the importing side we could go back and update the linkage for imported variables based on the linkage in the index after the IRMover importing is complete for the module (since the linkage in the index is currently ignored on the importing side). On the exporting side it would get internalized as usual during the index-based internalization. This has the advantage of being more consistent with how we currently internalize based on thin link analysis. Would that not work as expected?

• ReadOnly attribute's been moved to GlobalVarSummary and is calculated on analysis phase
• Added flag to ValueInfo which indicates "constantness" instead of using constant reference counter in FunctionSummary
• Fixed incremental build
• Fixed few issues including GV to GV references
• Fixed bitcode tests and added extra test cases

In fact I think that making this change to shouldPromoteLocalToGlobal on the importing side to get the linkage type from the index would work today

Finally I have some time to work on this again
I've simplified a patch to a large extent and tested it on a number of real applications (see below)

Yeah, I'm also wondering what's this for.

Are those ever expected to reference a blockaddress?