Page MenuHomePhabricator

[Support] Remove LLVM_ATTRIBUTE_NORETURN
ClosedPublic

Authored by gAlfonso-bit on Jul 27 2021, 11:15 AM.

Details

Reviewers
MaskRay
jhenderson
Group Reviewers
Restricted Project
Commits
rG09529892b518: [Support] Remove LLVM_ATTRIBUTE_NORETURN
Summary

Code should use C++11 [[noreturn]] or C11 _Noreturn instead.

Diff Detail

Event Timeline

gAlfonso-bit created this revision.Jul 27 2021, 11:15 AM
gAlfonso-bit requested review of this revision.Jul 27 2021, 11:15 AM
gAlfonso-bit retitled this revision from [LLVM] Add C++ 14 and C11 noreturn specifier to [LLVM] Use C++ 14 and C11 noreturn specifier.Jul 27 2021, 11:17 AM

I had to format my changes using clang format, which is why the file has formatting changes.

I would like to see you undo all the formatting changes.
They are not germane for this patch, and (IMHO) reduce the readability of the code.

Removed unneeded formatting changes

I would like to see you undo all the formatting changes.
They are not germane for this patch, and (IMHO) reduce the readability of the code.

Done!

gAlfonso-bit updated this revision to Diff 362154.EditedJul 27 2021, 1:12 PM

Fixed typo

Done!

Thank you. That's *much* easier to review.

I am not an approver for LLVM, but this LGTM.

I am not an approver for LLVM, but this LGTM.

Who is though?

gAlfonso-bit retitled this revision from [LLVM] Use C++ 14 and C11 noreturn specifier to [LLVM][NFC] Use C++ 14 and C11 noreturn specifier.Jul 27 2021, 2:16 PM

Had to fix an attribute list compile error due to the expanded macro

@MaskRay could you please look at this when you get the chance?

Fix more compilation errors

Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 6:02 PM

Every touched file that isn't Compiler.h had to be slightly tweaked (just word order change in function declarations) in order for the compilation to go through without any errors

MaskRay requested changes to this revision.EditedJul 27 2021, 6:53 PM

[[noreturn]] is C++11.
In Oct 2016, the minimum compiler requirement was raised to GCC 4.8/MSVC 2015.
We can use [[noreturn]] since then.

I migrated lld/. I will watch the build bots and migrate llvm/

This revision now requires changes to proceed.Jul 27 2021, 6:53 PM

I did that for you. Did a search and replace. Now we don't need the keyword anymore!

Removed LLVM_ATTRIBUTE_NORETURN entirely since the project's minimum requirements now allow us to use [[noreturn]] directly.

Herald added a project: Restricted Project. · View Herald Transcript
gAlfonso-bit retitled this revision from [LLVM][NFC] Use C++ 14 and C11 noreturn specifier to [LLVM][NFC] Remove LLVM_ATTRIBUTE_NORETURN and use [[noreturn]] directly.Jul 28 2021, 7:20 AM

Formatted ONLY the code I changed

@MaskRay I finished the migration to noreturn

gAlfonso-bit added a comment.EditedJul 28 2021, 8:35 AM

Also, I rebased my fork to LLVM 13 Release Branch so they don't clash with your changes

This comment was removed by gAlfonso-bit.
MaskRay added a comment.EditedJul 28 2021, 9:39 AM

I migrated all llvm/ files. You need to remove them from the patch.

It is a good idea to drop the macro definition in llvm/include/llvm/Support/Compiler.h separately.

It makes it easy to bring back the definition if some important downstream users want extended time for migration.
(Note: the policy is that downstream is on their own. Our thinking of this is kindness, not an obligation of llvm-project.)
For example, swift is using LLVM_ATTRIBUTE_NORETURN.
(I researched a bit: LLVM_ATTRIBUTE_NORETURN is not common in downstream projects.)

I migrated all llvm/ files. You need to remove them from the patch.

It is a good idea to drop the macro definition in llvm/include/llvm/Support/Compiler.h separately.

It makes it easy to bring back the definition if some important downstream users want extended time for migration.
(Note: the policy is that downstream is on their own. Our thinking of this is kindness, not an obligation of llvm-project.)
For example, swift is using LLVM_ATTRIBUTE_NORETURN.
(I researched a bit: LLVM_ATTRIBUTE_NORETURN is not common in downstream projects.)

Alright, but can we cherry pick this to release when this is all said and done?

gAlfonso-bit updated this revision to Diff 362492.EditedJul 28 2021, 12:27 PM

Fixed clang format

MaskRay accepted this revision.Jul 28 2021, 1:29 PM

Drop llvm/include/llvm/Support/Compiler.h change.

This revision is now accepted and ready to land.Jul 28 2021, 1:29 PM
gAlfonso-bit retitled this revision from [LLVM][NFC] Remove LLVM_ATTRIBUTE_NORETURN and use [[noreturn]] directly to [LLVM][NFC] Replace LLVM_ATTRIBUTE_NORETURN with [[noreturn]].

Removed Compiler.h changes

This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362780.EditedJul 29 2021, 7:53 AM

Had to fix a clang-tidy issue for this to even compile, because buildbot was throwing a fit about it

Still not passing (what is going on?)

This revision was landed with ongoing or failed builds.Jul 29 2021, 10:10 AM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [LLVM][NFC] Replace LLVM_ATTRIBUTE_NORETURN with [[noreturn]] to [Support] Remove LLVM_ATTRIBUTE_NORETURN.Jul 29 2021, 10:11 AM
MaskRay edited the summary of this revision. (Show Details)

Still not passing (what is going on?)

If you click a harbomaster URI, sometimes the builds are good

x64 debian passed
x64 windows passed

and sometimes there may be failures apparently unrelated to your patch.
Some discretion is needed.

I ended up making the refactoring by myself. Your change included some whole-file clang-format formatting which wasn't suitable.
I usually run git diff -U0 --no-color 'HEAD^' | clang/tools/clang-format/clang-format-diff.py -i -p1 to only format related lines.