I wonder how many external C++ symbols we have that are not in a namespace. If it's not many, maybe having a warning for it in our clang plugin would be a way to avoid future ODR violations?

Not sure how feasible this would be, just a thought.

While checking for such types is good on its own, it does not directly fixes a possibility of such ODR detections: first of all, the types being external is not a prerequisite for the bug: I have seen it with hidden types as well, because the problem happens on while LLVM merges modules.

Second, global namespace is also not necessary. My example from https://llvm.org/bugs/show_bug.cgi?id=28930 can be modified, so that both classes live in bar namespace, and it will hit segfault just like with global namespace.

What matters is the collision for the type names. My current idea is to compile Chrome with LTO, then take all the .o files and extract type names from each of them, trying to detect collisions. It's not trivial, as we'll need to detect not only the fact a type is used in an object file, but also detect if it's the same or different type.

Clang plugin might be a good place to put a regression check after a cleanup, but it probably makes sense to start with a simpler ad-hoc tool at first.

I have implemented a utility to find such ODR violations. It takes LLVM IR object files, parses metadata and then matches them all.

At this time, there's some amount of false positives, which I would like to handle later, so I don't post the whole list. Instead, here is a few cherry-picked examples:

=== blink::UnacceleratedSurfaceFactory:
DW_TAG_class_type at ../../third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp?q=UnacceleratedSurfaceFactory&sq=package:chromium&l=426&dr=CSs

DW_TAG_class_type at ../../third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp?q=UnacceleratedSurfaceFactory&sq=package:chromium&l=800&dr=CSs

That looks like a copy paste, and the implementation happens to be the same, but it feels like a time bomb. Probably, worth moving into anonymous namespaces in the same files.


=== blink::LogicalExtent:
DW_TAG_enumeration_type at ../../third_party/WebKit/Source/core/css/CSSProperty.cpp
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSProperty.cpp?q=blink::LogicalExtent&sq=package:chromium&l=126&dr=Ss

DW_TAG_enumeration_type at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBlock.cpp?q=blink::LogicalExtent&sq=package:chromium&l=177&dr=Ss

Again, the same impl, but needs to be hidden into anonymous namespace.


=== blink::stringpool_t:
DW_TAG_structure_type at gen/blink/core/CSSPropertyNames.cpp
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/CSSPropertyNames.cpp?q=stringpool_t+CSSPropertyNames.cpp&sq=package:chromium&l=1602&dr=CSs

DW_TAG_structure_type at gen/blink/core/CSSValueKeywords.cpp
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/CSSValueKeywords.cpp?dr=Ss&q=blink::stringpool_t&sq=package:chromium&l=1866

And these structs are actually different.

A few more:

=== ::CmpEquivLevel:
DW_TAG_structure_type at ../../third_party/icu/source/common/unormcmp.cpp
DW_TAG_structure_type at ../../third_party/icu/source/common/ustrcase.cpp


=== ::syllable_type_t:
DW_TAG_enumeration_type at ../../third_party/harfbuzz-ng/src/hb-ot-shape-complex-indic.cc
DW_TAG_enumeration_type at ../../third_party/harfbuzz-ng/src/hb-ot-shape-complex-myanmar.cc
DW_TAG_enumeration_type at ../../third_party/harfbuzz-ng/src/hb-ot-shape-complex-use.cc


=== ::yy_buffer_state:
DW_TAG_structure_type at Tokenizer.cpp
DW_TAG_structure_type at glslang_lex.cpp


=== ::yyalloc:
DW_TAG_union_type at ../../third_party/angle/src/compiler/preprocessor/ExpressionParser.cpp
DW_TAG_union_type at gen/blink/core/XPathGrammar.cpp


=== ::yyguts_t:
DW_TAG_structure_type at Tokenizer.cpp
DW_TAG_structure_type at glslang_lex.cpp

There's also ::event, which is a class and a struct at the same time:

=== ::event:
DW_TAG_class_type at ../../third_party/pdfium/fpdfsdk/javascript/event.h
https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/javascript/event.h?dr&q=third_party/pdfium/fpdfsdk/javascript/event.h&sq=package:chromium&l=12

DW_TAG_structure_type at ../../base/third_party/libevent/event.h
https://cs.chromium.org/chromium/src/base/third_party/libevent/event.h?q=struct.event+file:third_party/libevent/event.h&sq=package:chromium&l=213&dr=C

I am not even sure what to do here, as these type definitions are in header files, and there could be C++ files with both included (directly or indirectly).

Most likely, they will need to be moved out of a global namespace.

You probably already know that, but I thought that I'd still share that ASAN can also detect (some?) ODR violations and has found bugs in this area before - e.g.  issue 636563 .

Yes, but ASAN detects runtime ODR violation (like when you load a shared library). Here we have link-time ODRs and they may cause the linker to miscompile the code long before ASAN can have a chance to say something (but again -- in the case of a single binary it won't tell anything, as the linker would have already merged all the symbols for which ODR violations exist)

noticed this randomly, reassigning to pcc@ because krasin@ visited >30 days ago (very sad truth).