This is an archive of the discontinued LLVM Phabricator instance.

Differential D106899

[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
Herald added a subscriber: dexonsmith. · View Herald TranscriptJul 27 2021, 11:15 AM
gAlfonso-bit requested review of this revision.Jul 27 2021, 11:15 AM
Herald added a subscriber: llvm-commits. · View Herald TranscriptJul 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
gAlfonso-bit added a comment.Jul 27 2021, 12:45 PM
This comment was removed by gAlfonso-bit.
mclow.lists added a subscriber: mclow.lists.Jul 27 2021, 12:47 PM

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.

gAlfonso-bit updated this revision to Diff 362153.Jul 27 2021, 1:10 PM
This comment was removed by gAlfonso-bit.
gAlfonso-bit added a comment.Jul 27 2021, 1:10 PM
This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362154.Jul 27 2021, 1:12 PM
This comment was removed by gAlfonso-bit.
mclow.lists added a comment.Jul 27 2021, 1:33 PM
In D106899#2908395, @gAlfonso-bit wrote:

Done!

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

mclow.lists added a comment.Jul 27 2021, 1:35 PM

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

gAlfonso-bit added a comment.Jul 27 2021, 1:37 PM
This comment was removed by gAlfonso-bit.
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
Harbormaster completed remote builds in B116524: Diff 362154.Jul 27 2021, 3:27 PM
gAlfonso-bit updated this revision to Diff 362223.Jul 27 2021, 4:30 PM
This comment was removed by gAlfonso-bit.
Herald added a reviewer: MaskRay. · View Herald TranscriptJul 27 2021, 4:30 PM
gAlfonso-bit added a comment.Jul 27 2021, 5:47 PM
This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362250.Jul 27 2021, 6:02 PM
This comment was removed by gAlfonso-bit.
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 6:02 PM
Herald added subscribers: dcaballe, cota, teijeong and 18 others. · View Herald Transcript
gAlfonso-bit updated this revision to Diff 362253.Jul 27 2021, 6:05 PM
gAlfonso-bit added a comment.Jul 27 2021, 6:09 PM
This comment was removed by gAlfonso-bit.
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
Harbormaster completed remote builds in B116595: Diff 362253.Jul 27 2021, 8:25 PM
gAlfonso-bit added a comment.Jul 28 2021, 7:17 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362371.Jul 28 2021, 7:20 AM
This comment was removed by gAlfonso-bit.
Herald added a reviewer: jhenderson. · View Herald TranscriptJul 28 2021, 7:20 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: cfe-commits, ormris, lxfind and 5 others. · 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
gAlfonso-bit updated this revision to Diff 362398.Jul 28 2021, 8:13 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit added a comment.Jul 28 2021, 8:16 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit added a comment.Jul 28 2021, 8:35 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit added a comment.Jul 28 2021, 8:44 AM
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.)

gAlfonso-bit updated this revision to Diff 362437.Jul 28 2021, 10:07 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit added a comment.Jul 28 2021, 10:08 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362492.Jul 28 2021, 12:27 PM
This comment was removed by gAlfonso-bit.
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
Harbormaster completed remote builds in B116765: Diff 362492.Jul 28 2021, 1:42 PM
gAlfonso-bit updated this revision to Diff 362533.Jul 28 2021, 1:58 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

Harbormaster completed remote builds in B116799: Diff 362533.Jul 28 2021, 7:12 PM
gAlfonso-bit added a comment.Jul 29 2021, 6:36 AM
This comment was removed by gAlfonso-bit.
gAlfonso-bit updated this revision to Diff 362780.Jul 29 2021, 7:53 AM
This comment was removed by gAlfonso-bit.
Harbormaster completed remote builds in B116981: Diff 362780.Jul 29 2021, 9:01 AM
gAlfonso-bit added a comment.Jul 29 2021, 9:11 AM
This comment was removed by gAlfonso-bit.
This revision was landed with ongoing or failed builds.Jul 29 2021, 10:10 AM
Closed by commit rG09529892b518: [Support] Remove LLVM_ATTRIBUTE_NORETURN (authored by gAlfonso-bit, committed by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.
MaskRay added a commit: rG09529892b518: [Support] Remove LLVM_ATTRIBUTE_NORETURN.
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)
MaskRay added a comment.Jul 29 2021, 10:15 AM
In D106899#2913925, @gAlfonso-bit wrote:

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.

gAlfonso-bit added a comment.Jul 29 2021, 10:17 AM
This comment was removed by gAlfonso-bit.

Revision Contents

PathSize
llvm/
include/
llvm/
Support/
8 lines

Diff 362815

llvm/include/llvm/Support/Compiler.h

Show First 20 Lines • Show All 236 Lines • ▼ Show 20 Lines
#if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0) #if __has_attribute(always_inline) || LLVM_GNUC_PREREQ(4, 0, 0)
#define LLVM_ATTRIBUTE_ALWAYS_INLINE inline __attribute__((always_inline)) #define LLVM_ATTRIBUTE_ALWAYS_INLINE inline __attribute__((always_inline))
#elif defined(_MSC_VER) #elif defined(_MSC_VER)
#define LLVM_ATTRIBUTE_ALWAYS_INLINE __forceinline #define LLVM_ATTRIBUTE_ALWAYS_INLINE __forceinline
#else #else
#define LLVM_ATTRIBUTE_ALWAYS_INLINE inline #define LLVM_ATTRIBUTE_ALWAYS_INLINE inline
#endif #endif
#ifdef __GNUC__
#define LLVM_ATTRIBUTE_NORETURN __attribute__((noreturn))
#elif defined(_MSC_VER)
#define LLVM_ATTRIBUTE_NORETURN __declspec(noreturn)
#else
#define LLVM_ATTRIBUTE_NORETURN
#endif
#if __has_attribute(returns_nonnull) || LLVM_GNUC_PREREQ(4, 9, 0) #if __has_attribute(returns_nonnull) || LLVM_GNUC_PREREQ(4, 9, 0)
#define LLVM_ATTRIBUTE_RETURNS_NONNULL __attribute__((returns_nonnull)) #define LLVM_ATTRIBUTE_RETURNS_NONNULL __attribute__((returns_nonnull))
#elif defined(_MSC_VER) #elif defined(_MSC_VER)
#define LLVM_ATTRIBUTE_RETURNS_NONNULL _Ret_notnull_ #define LLVM_ATTRIBUTE_RETURNS_NONNULL _Ret_notnull_
#else #else
#define LLVM_ATTRIBUTE_RETURNS_NONNULL #define LLVM_ATTRIBUTE_RETURNS_NONNULL
#endif #endif
▲ Show 20 Lines • Show All 292 Lines • Show Last 20 Lines