This is an archive of the discontinued LLVM Phabricator instance.

[MS ABI]: Fix mangling function arguments for template types to be compatible with MSVC
ClosedPublic

Authored by adamf on Sep 26 2019, 12:32 PM.

Details

Summary

MS name mangling supports cache for first 10 distinct function arguments.
The error was when non cached template type occurred twice (e.g. 11th and 12th).
For such case in code there is another cache table TemplateArgStrings (for performance reasons).
Then one '@' character at the end of the mangled name taken from this table was missing.
For other cases the missing '@' character was added in mangleSourceName function.

Diff Detail

Repository
rC Clang

Event Timeline

adamf created this revision.Sep 26 2019, 12:32 PM
adamf edited the summary of this revision. (Show Details)Sep 26 2019, 12:34 PM
rnk accepted this revision.Oct 4 2019, 11:45 AM

lgtm

I confirmed the test case matches the name that MSVC produces. I guess this regressed in rC362293 / D62746, so the bug is in clang 9.0, but not 8.0. Maybe we should merge this to 9.0.1.

This revision is now accepted and ready to land.Oct 4 2019, 11:45 AM

(It regressed in D62780; I had bisected that bit.)

thakis accepted this revision.Oct 8 2019, 11:36 AM

lgtm. I finally got around to understanding the patch. Sorry for the delay, thanks for the fix! Next time you upload a patch, please upload it with more context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Note to self: The mangleSourceName() call mentioned in the patch description is 14 lines further up. mangleSourceName(TemplateMangling); outputs the number if there's a backref, else it outputs the argument followed by a '@'. Since we only cache mangleSourceName()'s argument, we need to append '@' ourselves. It seems a tiny bit nicer to store the '@' in the cache, but as-is is fine too.

Do you need someone to commit this for you?

adamf added a comment.Oct 8 2019, 11:46 AM

@thakis Yes please, I cannot commit this patch by myself, because I don't have commit access.
Surely, next time I will add more context in diff.
Thanks a lot for the effort.

This revision was automatically updated to reflect the committed changes.