[PATCH] D97436: [lld-link] Fix addrsig symbols merging in ICF.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Some nits.

I'm a bit worried that our end state for Chrome is going to be slightly confusing. In order to allow users to suppress ICF with attributes, this is what we will have to do:

- Add a new mode to clang -faddrsig that stops putting address taken functions in .llvm_addrsig (-faddrsig-data(-only)? -fno-addrsig-code?)
- Add a new attribute to clang that puts a function in .llvm_adddrsig (not sure if this should be called literally `__attribute__((addrsig))` or something general like `noicf`)
- Switch from /OPT:IFC to /OPT:SAFEICF in Chrome

Someone reading the build files might think that we are using safe ICF, but the compiler flag (-fno-addrsig-code or whatever) will mean we are effectively using regular, aggressive ICF. I guess we'll just have to live with that. It's pretty normal for build files to be confusing. =/



================
Comment at: lld/COFF/Config.h:86
+  Safe, // Safe ICF for all sections.
+  All,  // Default MSVC linker behavior.
+};
----------------
This isn't precisely MSVC's behavior. LLD will do ICF for code like MSVC, but it will do safe ICF for data using .llvm_addrsig, which MSVC doesn't look at. I'd make the comment more explicit, like: "Aggressive ICF for code, but safe ICF for data, similar to MSVC's behavior"

It seems to me that MSVC will not merge any readonly data sections:
```
extern "C" int printf(const char *, ...);
extern const int g1;
extern const int g2;
const int g1 = 42;
const int g2 = 42;
int main() {
  printf("g1: %d, g2: %d\n", g1, g2);
  printf("&g1: %p, &g2: %p\n", &g1, &g2);
}
```

g1 and g2 have distinct addresses:
```
$ cl -Z7 -Gy -Gw -O1 -c t.cpp -Facl.asm && link t.obj  -OPT:REF,ICF -debug && ./t.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29336 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
Microsoft (R) Incremental Linker Version 14.28.29336.0
Copyright (C) Microsoft Corporation.  All rights reserved.

g1: 42, g2: 42
&g1: 00007FF77EDD22E0, &g2: 00007FF77EDD22E4
```


================
Comment at: lld/COFF/ICF.cpp:87-88
   // Code sections are eligible.
   if (c->getOutputCharacteristics() & llvm::COFF::IMAGE_SCN_MEM_EXECUTE)
-    return true;
+    return icfLevel == ICFLevel::Safe ? !c->keepUnique : true;
 
----------------
nit: I feel like this would read better as:
  // Under regular (not safe) ICF, all code sections are eligible.
  if (icfLevel == ICFLevel::All && ...EXECUTE)
    return true;

I think that's functionally equivalent: under safe ICF we'd fall through to the main return.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97436/new/

https://reviews.llvm.org/D97436