[clang] Correct behavior of LLVM_UNREACHABLE_OPTIMIZE=OFF for Release builds

[MasterAwesome]

Codegen

Before

AArch64SVEPcsAttr *AArch64SVEPcsAttr::CreateImplicit(ASTContext &Ctx, SourceRange Range, Spelling S) {
  AttributeCommonInfo I(Range, NoSemaHandlerAttribute, (
    S == GNU_aarch64_sve_pcs ? AttributeCommonInfo::Form{AttributeCommonInfo::AS_GNU, GNU_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/} :
    S == CXX11_clang_aarch64_sve_pcs ? AttributeCommonInfo::Form{AttributeCommonInfo::AS_CXX11, CXX11_clang_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/} :
    S == C23_clang_aarch64_sve_pcs ? AttributeCommonInfo::Form{AttributeCommonInfo::AS_C23, C23_clang_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/} :
    (llvm_unreachable("Unknown attribute spelling!"),  AttributeCommonInfo::Form{AttributeCommonInfo::AS_GNU, 0, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/})));
  return CreateImplicit(Ctx, I);
}

After

AArch64SVEPcsAttr *AArch64SVEPcsAttr::CreateImplicit(ASTContext &Ctx, SourceRange Range, Spelling S) {
  AttributeCommonInfo I(Range, NoSemaHandlerAttribute, [&]() {
    switch (S) {
    case GNU_aarch64_sve_pcs:
      return AttributeCommonInfo::Form{AttributeCommonInfo::AS_GNU, GNU_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/};
    case CXX11_clang_aarch64_sve_pcs:
      return AttributeCommonInfo::Form{AttributeCommonInfo::AS_CXX11, CXX11_clang_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/};
    case C23_clang_aarch64_sve_pcs:
      return AttributeCommonInfo::Form{AttributeCommonInfo::AS_C23, C23_clang_aarch64_sve_pcs, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/};
    default:
      llvm_unreachable("Unknown attribute spelling!");
      return AttributeCommonInfo::Form{AttributeCommonInfo::AS_GNU, 0, false /*IsAlignas*/, false /*IsRegularKeywordAttribute*/};
    }
  }());
  return CreateImplicit(Ctx, I);
}

Fixes #68237

[AaronBallman]

Oh wow, that bug has been around for quite a while. I like your solution to it!

The code LGTM, no tests required because there isn't really a decent way to test this from lit. However, it would be nice to add a release note to clang/docs/ReleateNotes.rst to tell users that the bug was fixed. I'd add the note to the Bug Fixes in This Version section in the document.

[erichkeane]

Agree with Aaron on all points!

[MasterAwesome]

@erichkeane @AaronBallman, I've added the release notes and rebased the change on main. Does this need to be backported to 17.x? 16.x doesn't have this issue

[AaronBallman]

LGTM modulo Erich's comment on the release notes.

[MasterAwesome]

@AaronBallman, @erichkeane; whenever ya'll are done with the review, can you merge this in? I don't think I have access. And about the backporting, do we need it for 17.0.x bugfix releases?

[AaronBallman]

LGTM

[AaronBallman]

@erichkeane @AaronBallman, I've added the release notes and rebased the change on main. Does this need to be backported to 17.x? 16.x doesn't have this issue

Yeah, I think we should probably backport this to 17.x. We have instructions on how to kick that off here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches but I can get the ball rolling by adding the magic comment to the issue.

[MasterAwesome]

@erichkeane @AaronBallman, I've added the release notes and rebased the change on main. Does this need to be backported to 17.x? 16.x doesn't have this issue

Yeah, I think we should probably backport this to 17.x. We have instructions on how to kick that off here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches but I can get the ball rolling by adding the magic comment to the issue.

Thanks for doing it! I'll work on cherry-picking this to Rust's fork of llvm :)

[AaronBallman]

@erichkeane @AaronBallman, I've added the release notes and rebased the change on main. Does this need to be backported to 17.x? 16.x doesn't have this issue

Yeah, I think we should probably backport this to 17.x. We have instructions on how to kick that off here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches but I can get the ball rolling by adding the magic comment to the issue.

Thanks for doing it! I'll work on cherry-picking this to Rust's fork of llvm :)

You're welcome, but it seems there's a problem with trying to cherry-pick this over to 17.x and the pick will be more involved because of merge conflicts with the release notes. Would you mind taking over the pick to 17.x? If not, that's okay! (I'm mostly asking because I'm pretty swamped with reviews and I don't want the backport to fall through the cracks by accident.)

[MasterAwesome]

@erichkeane @AaronBallman, I've added the release notes and rebased the change on main. Does this need to be backported to 17.x? 16.x doesn't have this issue

Yeah, I think we should probably backport this to 17.x. We have instructions on how to kick that off here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches but I can get the ball rolling by adding the magic comment to the issue.

Thanks for doing it! I'll work on cherry-picking this to Rust's fork of llvm :)

You're welcome, but it seems there's a problem with trying to cherry-pick this over to 17.x and the pick will be more involved because of merge conflicts with the release notes. Would you mind taking over the pick to 17.x? If not, that's okay! (I'm mostly asking because I'm pretty swamped with reviews and I don't want the backport to fall through the cracks by accident.)

I gotchu! The PR created by llvmbot is at llvm/llvm-project-release-prs#726