Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ms mangler for template values: poor nyi error messages and nyi opte pointer #97756

Closed
memory-thrasher opened this issue Jul 4, 2024 · 5 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@memory-thrasher
Copy link
Contributor

I am reporting this bug for completeness. I am actively working on a patch submission.

The official MSVC mangler lacks support for some c++ that is valid according to the standard, clang, and every decent compiler. Concerning template instance name mangling, specifically, there are presently 6 cases where the function MicrosoftCXXNameMangler::mangleTemplateArgValue gives up mangling and emits the rather unhelpful error:

cannot mangle this template argument yet

I improved that error message on my local build to tell which of the 6 possible cases I was running into. That case is when the non-type template parameter's value is an l-value point to one-past-the-end of some constexpr array. This case is not possible in c++ code that supports the official MSVC compiler, so consistency with MSVC is both impossible and uneeded. Furthermore, templates are not generally called across an ABI boudnary, but rather, each binary contains its own instances.

I arbitrarily chose a mangling pseudonym to represent this case. I foresee two potential future issues, though both seem to me to be highly unlikely and neither, any worse than failing to compile the code in the first place.

  • Compatibility between binaries output by clang using this patch, with binaries produced by some other compiler or future version of clang that might use a different arbitrary pseudonym. I doubt this case will ever happen because for the reasons already declared above; each binary should contain its own template instance with its own mangling rules.
  • Collision between two template function instances of the same template function where the template argument for each is the OPTE value for two different arrays, and that is the only difference between the instances, and yet the two instances should have different behavior. I cannot think of a good reason the two functions should have different behavior when handed a pointer that is essentially invalid (does not point to a usable resource). Even a reverse iterator has better solutions.

The patch will include a minimal test case.

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Jul 4, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 4, 2024

@llvm/issue-subscribers-clang-frontend

Author: None (memory-thrasher)

## I am reporting this bug for completeness. I am actively working on a patch submission.

The official MSVC mangler lacks support for some c++ that is valid according to the standard, clang, and every decent compiler. Concerning template instance name mangling, specifically, there are presently 6 cases where the function MicrosoftCXXNameMangler::mangleTemplateArgValue gives up mangling and emits the rather unhelpful error:
> cannot mangle this template argument yet

I improved that error message on my local build to tell which of the 6 possible cases I was running into. That case is when the non-type template parameter's value is an l-value point to one-past-the-end of some constexpr array. This case is not possible in c++ code that supports the official MSVC compiler, so consistency with MSVC is both impossible and uneeded. Furthermore, templates are not generally called across an ABI boudnary, but rather, each binary contains its own instances.

I arbitrarily chose a mangling pseudonym to represent this case. I foresee two potential future issues, though both seem to me to be highly unlikely and neither, any worse than failing to compile the code in the first place.

  • Compatibility between binaries output by clang using this patch, with binaries produced by some other compiler or future version of clang that might use a different arbitrary pseudonym. I doubt this case will ever happen because for the reasons already declared above; each binary should contain its own template instance with its own mangling rules.
  • Collision between two template function instances of the same template function where the template argument for each is the OPTE value for two different arrays, and that is the only difference between the instances, and yet the two instances should have different behavior. I cannot think of a good reason the two functions should have different behavior when handed a pointer that is essentially invalid (does not point to a usable resource). Even a reverse iterator has better solutions.

The patch will include a minimal test case.

@shafik
Copy link
Collaborator

shafik commented Jul 4, 2024

A minimal test case would be helpful for the issue report as well.

@memory-thrasher
Copy link
Contributor Author

Here you go. Maybe not quite minimal but I wanted to make the usefulness when it works also apparent. Host system is ubuntu 22.04 so the default target uses itanium mangler. It's cross-compiling to msvc/windows that's broken.

ms_mangler_templatearg_opte.cpp.txt

@MaxEW707
Copy link
Contributor

MaxEW707 commented Jul 6, 2024

This case is not possible in c++ code that supports the official MSVC compiler, so consistency with MSVC is both impossible and uneeded. Furthermore, templates are not generally called across an ABI boundary, but rather, each binary contains its own instances.

For reference we were discussing this here: #94650.

While is this mostly true the original intention, afaik, is that -msvc triple is supposed to adhere to MSVC mangling including all its bugs. At least I regularly have code shipped to us from third-parties that are only built with MSVC and needs to link against clang-cl. We want the mangling to cohere even if it is to ensure we do not have duplicate symbols bloating a binary especially in debug builds (link times are a concern for us).
See #83616 and #85300 which recently also fixed clang not adhering to MSVC mangling among other fixes in MicrosoftMangle.cpp.

In general I do agree especially in your example that if clang has a custom mangling it probably will never be noticed in debug builds and in release builds where it will most likely be inlined. However if any of these methods have static locals then multiple definitions across objects built with msvc vs clang isn't ideal.

I am writing up a discourse to get consensus on this as was discussed in #94650. Hopefully that goes up sometime tomorrow but I am fairly certain the outcome will be -msvc triple adheres to MSVC and we may have a separate triple for those who want MSVC-like mangling behaviour especially for templates.

rnk pushed a commit that referenced this issue Jul 24, 2024
…to handle template argument values that are pointers one-past-the-end
of a non-array symbol. Also improves error messages in other template
argument scenarios where clang bails.

#97756

I don't think I hooked up the unit test right. I'm not sure one is
really needed for what boils down to a tweaked if statement. Please
advise.
@memory-thrasher
Copy link
Contributor Author

verified working on trunc after that pr was merged.

yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
Summary:
…to handle template argument values that are pointers one-past-the-end
of a non-array symbol. Also improves error messages in other template
argument scenarios where clang bails.

#97756

I don't think I hooked up the unit test right. I'm not sure one is
really needed for what boils down to a tweaked if statement. Please
advise.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

5 participants