Page MenuHomePhabricator

[Clang] add missing ClangABICompat check
Needs ReviewPublic

Authored by ychen on Thu, Sep 22, 5:54 PM.

Details

Summary

In the same spirit of da6187f566b7881cb. This maintains compatibility
with GCC at the C++ language level. Nothing is changed in the traditional ABI sense, i.e. layout/mangling etc.

Event Timeline

ychen created this revision.Thu, Sep 22, 5:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 22, 5:54 PM
ychen requested review of this revision.Thu, Sep 22, 5:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 22, 5:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Anytime there's a Clang ABI version check, I have to wonder whether PS4/PS5 need an exception. (And I'm sorry I missed the earlier patches in this series.) We are pretty much locked to however things worked for Clang 3.2.
Is this specific to C++20 (or later)? In that case the question gets more complicated because we don't yet support C++20. But I see the predecessors of this patch are referring to DRs, and as we all know the C++ committee loves to retroactively redefine earlier versions of the language...
So I don't know what to think at the moment. It's late and I don't have the mental energy to pursue this right at the moment.
I'll try to get back and look more closely tomorrow.

ychen edited the summary of this revision. (Show Details)Thu, Sep 22, 6:35 PM

Hi @ychen I saw you updated the patch description, thanks.
IIUC, the change causes some difference in the interpretation of particular template stuff. Would this be clearer if the BEFORE-15/AFTER-15 test cases used the same template construct? Currently they are different so it's hard to work out the effect of the change. In particular, naively I see different mangled names which makes me concerned that there _is_ in fact a net change to mangling.

I'll observe that D128745 updated the release notes, but not in the most useful way; it cites DRs without any explanation of what they do, and with no links.

ychen added a comment.Fri, Sep 23, 3:24 PM

Would this be clearer if the BEFORE-15/AFTER-15 test cases used the same template construct? Currently they are different so it's hard to work out the effect of the change. In particular, naively I see different mangled names which makes me concerned that there _is_ in fact a net change to mangling.

Understood. It is hard because the BEFORE-15 and AFTER-15 cases have compilation failures when the ClangABICompat switch is different and checking the IR requires it to compile cleanly. The mangling actually does not change for the same function type. If the mangling changes, then the (instantiated) function type is different at the souce-code level. It is a little bit different from the real ABI that the same source-code construct has a different binary representation (memory/symbol) etc.

I'll observe that D128745 updated the release notes, but not in the most useful way; it cites DRs without any explanation of what they do, and with no links.

Indeed. I'll submit a follow-up patch to improve that.

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

This was discussed here https://reviews.llvm.org/D128745#inline-1244757. Yeah, it is somewhat confusing to key the legacy language behavior on ClangABI. I'm not sure there are better choices than inventing new flags.

rjmccall added a comment.EditedTue, Sep 27, 11:48 AM

I don't think I agree that changes to template partial ordering rules are properly understood as breaking changes to the ABI. They're breaking changes to the *language behavior*, and the ABI break is downstream of that, just it would be if, say, adding two floats suddenly produced a double. I think the right approach for Clang is to gate this by language version and add a note somewhere in the documentation that C++23 changed the overload resolution rules around function templates and that this means adopting C++23 can be ABI-breaking. The same has been true of many other language revisions in the past. Fortunately, it should only be ABI-breaking (or source-breaking) in rare circumstances, which is presumably why the C++ committee felt it could get away with this change.

It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise.
@aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

This was discussed here https://reviews.llvm.org/D128745#inline-1244757. Yeah, it is somewhat confusing to key the legacy language behavior on ClangABI. I'm not sure there are better choices than inventing new flags.

Right, which points to https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L9786 concerning the treatment of copy constructors. But those at least can feed into how one goes about conjuring up calling sequences involving parameters of types that might have to invoke those constructors, and so that case seems a lot more ABI-relevant than subtle details about template parameter packs.

Anyway, I'm going to step out of this now, I don't have the expertise to review this responsibly.