Another possibility would be to consider clang's ODR checker, but I think it's a work in progress.

I have a concern about the performance of the --detect-odr-violations flag and wondering if it is fast enough to be always on.

If my understanding is correct, when the --detect-odr-violations flag is given, gold checks debug info for each duplicate definition to see if all the definitions are at the same line in the same file. I'd guess it could be very slow.

It did feel slower locally when linking with gold and --detect-odr-violations, but I did not actually measure. IIRC, we did enable it while Chromium was using GYP and I don't recall getting any complaints.

LLD is probably 3x faster than gold now, and that was achieved by carefully designing features with performance in mind. So, I'd like to set a higher bar for the --detect-odr-violations flag so that you can use the flag almost for free.

It is okay to get a help from clang if doing it makes sense. For example, I can imagine that we could emit a parallel array for a symbol table that contains "signature" of each symbol so that the linker emits a warning if these signatures don't match for the same symbol from different files.

Clang is growing support for generating "ODR hashes" for functions as part of the ODR checker I mentioned in comment 1. We could in principle teach clang to emit the ODR hashes in something like the signature table that you mentioned. This should hopefully be more efficient (since the work is moved to compile time) as well as more precise than matching based on debug info (e.g. you could identify ODR violations caused by different preprocessor output in different TUs).

I'm not sure whether ODR checking should be done on symbols, though. For example, we may also want to be able to detect ODR violations on type definitions, and if the ODR violation is in an inline function definition and it is inlined into every caller, we'd want to detect it but still drop the symbol. So we may want to create a separate "namespace" in the linker for ODR checking.

Could you give me a pointer to a document about the clang support of the "ODR hashes"?

I'm not sure whether there is a single document about this, but you can try searching cfe-dev/cfe-commits for "odrhash" for some related commits. Richard Trieu has been the main person working on this as I understand it.

The current ODRHashing scheme takes a CXXRecordDecl and computes a 32-bit hash based on the AST, but independent from pointers values, to make the hash consistent over runs.  Currently, it is only computed during modules-enabled builds to check that objects that are merged during modules import are the same.  There currently is not documentation as I am still working out some problems with the hash computation.  Let me know if there are any questions about this I can answer.

I don't have a specific question at the moment, but I think emitting that hashes to object files and using it from the linker is the way to go, as it could make ODR violation checking significantly cheaper than the one using debug info.

That implies LLD's ODR violation checking would need Clang, but I'm not too worried about that. Rather, I'd think this is a good example what we can do if we can change both the linker and the compiler.

I hacked up a prototype ODR checker here:
https://github.com/pcc/llvm-project/tree/odr-checker
My change makes clang create a signature table in each object file containing the ODRHash of each record declaration in the TU, and makes lld read the signature tables in the input files and report ODRHash mismatches.

I ran my patched version of clang/lld over Chromium. It finds some genuine ODR violations, such as:

https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/include/leveldb/comparator.h?l=18
https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/db/skiplist_test.cc?l=17

https://cs.chromium.org/chromium/src/third_party/skia/src/effects/SkLightingImageFilter.cpp?l=189
https://cs.chromium.org/chromium/src/third_party/skia/src/effects/SkMatrixConvolutionImageFilter.cpp?l=122

It also finds stuff like this:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebCString.h?l=90
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebHTTPBody.h?l=108
which is presumably intentional. Are there any plans to remove the INSIDE_BLINK macro?

There are also a bunch of apparent false positives. For example, it thinks that this definition contains an ODR violation:
https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/parseerr.h?l=58
but I do not understand why. Maybe this is related to the bug in hash computation that Richard alluded to.

My conclusion is that this may be a useful contribution to LLVM once the change is cleaned up and the bugs are worked out. I'm not sure if I'll have any more time to work on this soon though.

In my local build using gold, the remaining issues are:
- tcmalloc's base atomic ops vs base atomic ops.
- swiftshader's copy of subzero has issues in IceGlobalContext.
- libphonenumber's RegionCode vs test RegionCode, when building the libphonenumber tests.

Not sure why it's finding a different set of issues as your checker.

Nice!

> Are there any plans to remove the INSIDE_BLINK macro?

iirc the very-long-term-plan is to get rid of the whole WebKit/public API, but that's a ways away.

> - tcmalloc's base atomic ops vs base atomic ops.
> - swiftshader's copy of subzero has issues in IceGlobalContext.

My checker did not find these. What sort of issues are they? My checker only works on struct/class declarations, which is a limitation of clang's ODR hasher I believe. It should be easy to extend to other types of declarations (such as functions) if clang is extended to provide that information.

> - libphonenumber's RegionCode vs test RegionCode, when building the libphonenumber tests.

Looks like it found this one.

I've attached a list of unique reports to the bug.

Deleted: odrs.txt 31.0 KB

odrs.txt 31.0 KB View Download

You can uncomment the --detect-odr-violations block in build/config/compiler/BUILD.gn and see for yourself. You can als look at the recent CLs associated with bug 449754 for some of the other ones I fixed.

- base::subtle::NoBarrier_CompareAndSwap() and friends are defined in both base/atomicops_internals_portable.h and third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.h.

ld.gold: warning: while linking ./libbase.so: symbol 'base::subtle::NoBarrier_CompareAndSwap(int volatile*, int, int)' defined in multiple places (possible ODR violation):
  ../../third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.h:74 from obj/base/allocator/tcmalloc/spinlock.o
  ../../base/atomicops_internals_portable.h:67 from obj/base/base/stack_sampling_profiler.o

- ld.gold: warning: while linking swiftshader/libGLESv2.so: symbol 'TLS wrapper function for Ice::GlobalContext::TLS' defined in multiple places (possible ODR violation):
  ../../third_party/swiftshader/third_party/subzero/src/IceDefs.h:121 from obj/third_party/swiftshader/src/Reactor/swiftshader_subzero/IceTargetLoweringX8664.o
  ../../third_party/swiftshader/third_party/subzero/src/IceGlobalContext.cpp:373 from obj/third_party/swiftshader/src/Reactor/swiftshader_subzero/IceGlobalContext.o

I ended up avoiding this one with https://codereview.chromium.org/2896133003/ but it hasn't been rolled in via DEPS yet.

That's a really great change! I didn't expect that an experimental feature can be implemented in such a small number of lines of code.

Maybe we should make the -detect-odr-violations a clang flag (as opposed to a linker flag) and let lld detect ODR violations whenever the information is available in input object files.

> There are also a bunch of apparent false positives. For example, it thinks that this definition contains an ODR violation:
> https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/parseerr.h?l=58
> but I do not understand why. Maybe this is related to the bug in hash computation that Richard alluded to.

Is it possible the UChar macro is different in each compile?  Being able to detect when the same source produces different classes was one of the goals of the ODR Hash.

> My conclusion is that this may be a useful contribution to LLVM once the change is cleaned up and the bugs are worked out. I'm not sure if I'll have any more time to work on this soon though.

It would be good to know if there are any requirements for ODR checking in the linker.  I have only tested and taken into consideration issues dealing with ODR and modules merging.

> ld.gold: warning: while linking ./libbase.so: symbol 'base::subtle::NoBarrier_CompareAndSwap(int volatile*, int, int)' defined in multiple places (possible ODR violation)

https://cs.chromium.org/chromium/src/third_party/tcmalloc/chromium/src/base/atomicops-internals-x86.h?l=74
https://cs.chromium.org/chromium/src/base/atomicops_internals_portable.h?l=67

Thanks, this looks like an inline function definition, which my checker doesn't support yet.

> ld.gold: warning: while linking swiftshader/libGLESv2.so: symbol 'TLS wrapper function for Ice::GlobalContext::TLS' defined in multiple places (possible ODR violation)

This is a bug in gold's checker, right? I don't see anything problematic there (unless I'm missing something).

> Maybe we should make the -detect-odr-violations a clang flag (as opposed to a linker flag) and let lld detect ODR violations whenever the information is available in input object files.

Right, that is exactly how I imagined the interface would look.

> Is it possible the UChar macro is different in each compile?

Oh right, good catch. I guess I just sort of assumed that UChar would be defined in a reasonable way.

> It would be good to know if there are any requirements for ODR checking in the linker.

From the ODRHash perspective, what currently exists is useful on its own, but it would be nice to have a way of checking ODR on inline function declarations (e.g. the atomic operations that thestig@ pointed out). Maybe I don't understand something about modules, but wouldn't you need ODR checking on inline functions for modules as well?

> Maybe I don't understand something about modules, but wouldn't you need ODR checking on inline functions for modules as well?

It's a work in progress, and classes seemed like a good place to start.  Class methods are checked by their signature only.  Checking for other functions and function bodies are not yet implemented.  And I think enums will need a separate hash too.

> It's a work in progress, and classes seemed like a good place to start.  Class methods are checked by their signature only.  Checking for other functions and function bodies are not yet implemented.  And I think enums will need a separate hash too.

I see, I didn't realise that function bodies were not being hashed yet. Enums would also be good for linker-based checking.

For reference here is an ODR violation that was committed and then detected by VC++'s LTCG (actually PGO) build:

https://chromium-review.googlesource.com/c/chromium/src/+/767287/6/content/renderer/dom_storage/session_storage_namespace.h

Here's another ODR violation that link.exe caught:  crbug.com/857277 

I was surprised that the duplicate function detection happened when the library was created rather than when it was used. This is particularly relevant to Windows now that we don't have link.exe to uncover some of these issues.

FWIW.

GenerateDomKeyboardLayoutMap doesn't seem like a COMDAT, so isn't this just a duplicate symbol error rather than a ODR violation?

From https://en.wikipedia.org/wiki/One_Definition_Rule:

> In the entire program, an object or non-inline function cannot have more than one definition

Having two definitions (two different definitions in this case) for one function is an ODR violation. I don't know whether it is the type of ODR violation that is being discussed here.