Compiler error message points to function template for instantiations with the same mangled name

[Quuxplusone]

Bugzilla Link	PR25343
Status	NEW
Importance	P normal
Reported by	Douglas Yung (douglas_yung@playstation.sony.com)
Reported on	2015-10-28 17:35:36 -0700
Last modified on	2019-11-25 03:54:26 -0800
Version	trunk
Hardware	PC Windows NT
CC	andrew.v.tischenko@gmail.com, duhowett@microsoft.com, llvm-bugs@lists.llvm.org, llvm-dev@redking.me.uk, paul_robinson@playstation.sony.com, richard-llvm@metafoo.co.uk, rjmccall@apple.com, steffen.seckler@tum.de, zahira.ammarguellat@intel.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

I am seeing a case where the compiler emits an error message which states the
compiler detected identical mangled names, but when pointing out the locations,
the duplicate and the previous definition point to the same incorrect line.

To reproduce this issue, compile the following code:

template <typename MathType, typename ElementType>
static void SplatConstructor1Test() {}

void foo() {
  SplatConstructor1Test<int, float __attribute__((__vector_size__(16)))>();
  SplatConstructor1Test<int, float __attribute__((ext_vector_type(4)))>();
}

When compiled, the compiler emits the following error messages:

repro.cpp:2:13: error: definition with same mangled name as another definition
static void SplatConstructor1Test() {}
            ^
repro.cpp:2:13: note: previous definition is here
1 error generated.

The error message should ideally direct the user to lines 5 and 6 for the
original definition and the duplicate respectively. I suspect what is happening
is that since these functions are template instantiations, the compiler creates
that code and creates the error, but since it is compiler generated code and
not user code, there really isn't a good place to report as the site of the
error to the user.

Perhaps there is some way the error message can be made better?

[Quuxplusone]

Ideally, the error message should reference the instantiations which result in the identically mangled names and point to lines 5 and 6 in the error message.

[Quuxplusone]

I'm curious about the state of this issue; In our codebase, clang trunk diagnoses identical mangled names that, if the diagnostic is removed, seem to cause no trouble. It looks like they're in two separate instantiations of the same class template. Since the diagnostic points at the same line, it's impossible to tell which is at fault.

These appears to be a chance that neither is at fault and the document is well-formed.

[Quuxplusone]

Our reduced repro points to types templated over Objective-C generics.

% cat test.mm
__attribute__((objc_root_class))
@interface One
@end
__attribute__((objc_root_class))
@interface Two
@end
__attribute__((objc_root_class))
@interface Root <__covariant Type>
@end

template <typename T>
struct F {
        using TPtr = T*;
        TPtr get() { return nullptr; }
};

void repro() {
        F<Root<One*>> one;
        F<Root<Two*>> two;

        one.get();
        two.get();
}

---

% & ".\clang.exe" --% -o - -S -emit-llvm .\test.mm
.\test.mm:14:7: error: definition with same mangled name as another definition
        TPtr get() { return nullptr; }
             ^
.\test.mm:14:7: note: previous definition is here
1 error generated.

[Quuxplusone]

Adding Richard And John who might be able to help.

[Quuxplusone]

This diagnostic is a last-ditch message from IRGen informing the user that the
compiler has detected something weird that it doesn't know how to reasonably
support, which is why it doesn't have the usual benefits of Sema's template
instantiation diagnostics.  It's inevitable that we will have this sort of
diagnostic, and it's inevitable that there will be cases where it's a pretty
bad diagnostic, but of course that doesn't mean we can't try to improve it
where we can.

The root cause of the problems in your examples is that we have multiple types
that Sema considers different types but that the mangler mangles the same way.
So you haven't found a "reduction", you've actually just found another instance
of a general class of bug.

I think there are three things to do here:

1. We can improve the last-ditch diagnostic to recognize that it's referencing
a template instantiation and include a pretty-print of the name of the
declaration with all its template arguments.  Something like:

  repro.cpp:2:13: error: definition with same mangled name as another definition
  repro.cpp:2:13: note: definition is template instantiation 'SplatConstructor1Test<int, float __attribute__((__vector_size__(16)))>'
  repro.cpp:2:13: note: previous definition is template instantiation 'SplatConstructor1Test<int, float __attribute__((ext_vector_type(4)))>'

2. The ideal fix to the vector-type problem would be to just make these the
same canonical type and just remember the spelling difference as sugar.  That
would require some semantic investigation, because I know there are some
differences in terms of what operations they provide, and if those seriously
conflict, a unification would never work.  If so, we'll either have to give
them different manglings or give up and accept the possibility of spurious
conflicts like this.

3. The best fix to the ObjC generics problem would be to mangle ObjC generic
arguments.  However, it's likely that Doug Gregor intentionally chose not to do
this when he implemented the feature; you'll need to talk to him.

[Quuxplusone]

(In reply to John McCall from comment #5)
> 2. The ideal fix to the vector-type problem would be to just make these the
> same canonical type and just remember the spelling difference as sugar.
> That would require some semantic investigation, because I know there are
> some differences in terms of what operations they provide, and if those
> seriously conflict, a unification would never work.  If so, we'll either
> have to give them different manglings or give up and accept the possibility
> of spurious conflicts like this.

If I remember correctly, there are some syntaxes (possibly involving braced
initialization?) that are valid but mean different things for the two kinds of
vectors. And there are syntaxes that are intentionally valid only for one kind
or the other. I expect we'll find that using two different manglings is the
only feasible solution.

[Quuxplusone]

Yeah, I know it's probably a fantasy; but our current vector language design is really, really bad.

[Quuxplusone]

I have some old notes saying the mangling issue was brought up on
cxx-abi-dev back in Feb 2016, although I don't think it was settled.
Here are a couple of possibly dead links (I can't bring up the pages):

http://sourcerytools.com/pipermail/cxx-abi-dev/2016-February/002897.html
http://sourcerytools.com/pipermail/cxx-abi-dev/2016-February/002900.html

[Quuxplusone]

I still have archived copies of those cxx-abi-dev messages. It looks like the
problem is:

 * GCC uses Dv manglings for __attribute__((__vector_size__))
 * Clang uses those manglings for both forms of vector
 * The SPIR spec appears to have just looked at what Clang did for ext_vector_type and defined that as their ABI

So we have a conflict: GCC compatibility requires that vector_size vectors be
given Dv manglings. SPIR compatibility requires that ext_vector_type vectors be
given Dv manglings.

From cxx-abi-dev:

"""
To recap, the suggestion was:
  When targeting SPIR, we mangle ext_vector_type as Dv<N>_<T>, where N is
  the number of elements in the vector, and T is the mangled element type;
  and we mangle vector_size as something else. This is consistent with
  SPIR 1.2 and SPIR 2.0 specs.

  When targeting other-than-SPIR, we mangle vector_size as Dv<N>_<T>;
  and we mangle ext_vector_type as something else. This is consistent with
  GCC 5.x.
"""

This is a pretty sad state of affairs, but seems workable (and is similar to
what we do for __float128 where a similar ABI collision has occurred).

But, like John, I want to see if we can fix Clang's proliferation of vector
types and vector semantics. This doesn't stop with just vector_size vs
ext_vector_type. At some point, someone will ask for (eg) Altivec semantics in
a program that also uses ARM Neon vectors.