[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone attribute. +// FIXME: we don't, though. // CHECK-LABEL: @t

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

svenvh added inline comments. Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20 // Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone attribute. +// FIXME: we don't, though. // CHECK-LABEL: @test_const_attr lebedev.ri wrote:

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. Ok, so. I looked really hard, and essentially i'm not sure we can just change those attributes from implying `readonly`/`readnone`. Things kinda just fall apart Comment at: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl:20 // Test that Attr.C

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

aaron.ballman added a comment. In D138958#4060633 , @lebedev.ri wrote: > Ok, if we must not unconditionally emit the memory attributes, then let's not. > Please stamp? :) FWIW, I don't think we should land any of this until after the Clang 16 branch ha

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri updated this revision to Diff 489975. lebedev.ri added subscribers: Anastasia, arsenm. lebedev.ri added a comment. Ok, if we must not unconditionally emit the memory attributes, then let's not. Please stamp? :) @Anastasia @venvh @arsenm This breaks opencl headers in a way i do not und

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. @jdoerfert can you please clarify, do you believe we are fine as-is, or need to change attributes we emit? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138958/new/ https://reviews.llvm.org/D138958

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

nlopes added a comment. The issue is the function call, not the stores. Stores are valid as long as they are to local memory (stack or allocated by the function). Because of the call, you can't tag it as `memory(none)`. But you may be able to tag it with inaccessiblemem if the call is also mark

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added subscribers: nlopes, regehr. lebedev.ri added a comment. In D138958#4054903 , @jdoerfert wrote: > In D138958#4045567 , @efriedma > wrote: > >> From an IR semantics standpoint, I'm not sure the `m

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

jdoerfert added a comment. In D138958#4045567 , @efriedma wrote: > From an IR semantics standpoint, I'm not sure the `memory(none)` marking is > right if the function throws an exception. LangRef doesn't explicitly > exclude the possibility, but take t

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

efriedma added a comment. I'm not trying to argue what the `pure`/`const` attribute in C/C++ is supposed to mean. I agree with your interpretation of the gcc documentation/implementation. I'm saying that there's a mismatch between the gcc `pure`/`const` and the LLVM IR `memory(read)`/`memory(

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. In D138958#4045567 , @efriedma wrote: > From an IR semantics standpoint, I'm not sure the `memory(none)` marking is > right if the function throws an exception. Then `memory(read)` would be wrong, either. A bit of a hot take:

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

efriedma added subscribers: nikic, efriedma. efriedma added reviewers: nikic, jdoerfert. efriedma added a comment. From an IR semantics standpoint, I'm not sure the `memory(none)` marking is right if the function throws an exception. LangRef doesn't explicitly exclude the possibility, but take

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri updated this revision to Diff 488343. lebedev.ri marked an inline comment as done. lebedev.ri edited the summary of this revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138958/new/ https://reviews.llvm.org/D138958 Files: clang-t

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4138 + "functions and statements">; + let SimpleHandler = 1; +} lebedev.ri wrote: > aaron.ballman wrote: > > lebedev.ri wrote: > > > aaron.ballman wrot

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri marked 2 inline comments as done. lebedev.ri added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4138 + "functions and statements">; + let SimpleHandler = 1; +} aaron.ballman wrote: > lebedev.ri wrote: > >

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4138 + "functions and statements">; + let SimpleHandler = 1; +} lebedev.ri wrote: > aaron.ballman wrote: > > lebedev.ri wrote: > > > aaron.ballman wrot

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri marked 2 inline comments as done. lebedev.ri added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4138 + "functions and statements">; + let SimpleHandler = 1; +} aaron.ballman wrote: > lebedev.ri wrote: > >

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4133 +def NoUnwind : DeclOrStmtAttr { + let Spellings = [Clang<"nounwind", /*allowInC =*/0>]; + let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>]; lebed

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4133 +def NoUnwind : DeclOrStmtAttr { + let Spellings = [Clang<"nounwind", /*allowInC =*/0>]; + let Accessors = [Accessor<"isClangNoUnwind", [CXX11<"clang", "nounwind">]>]; aaron.ba

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri updated this revision to Diff 487933. lebedev.ri marked 10 inline comments as done. lebedev.ri added a comment. @aaron.ballman thank you for taking a look! Addressed all review notes other than the SEH question and the simplehandler one. (i'll deal with whitespace changes when committi

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

aaron.ballman added a comment. Precommit CI seems to have found relevant issues that need to be addressed. Comment at: clang/include/clang/Basic/Attr.td:4133 +def NoUnwind : DeclOrStmtAttr { + let Spellings = [Clang<"nounwind", /*allowInC =*/0>]; + let Accessors = [Accessor<"

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:565 +returning a value. They can not write to memory, but may read memory that is +immutable between invocations of the function. + lebedev.ri wrote: > erichkeane wrote: > > A qui

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:565 +returning a value. They can not write to memory, but may read memory that is +immutable between invocations of the function. + erichkeane wrote: > A quick sentence somewhere

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri updated this revision to Diff 487548. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. In D138958#4037526 , @erichkeane wrote: > 2 quick nits, otherwise LFTM. @erichkeane thank you for the review! @aaron.ballman would

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 2 quick nits, otherwise LFTM. Comment at: clang/include/clang/Basic/AttrDocs.td:558 + let Content = [{ +A stronger version of ``__attribute__((pure))`` attribute. +

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. Herald added a subscriber: StephenFan. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138958/new/ https://reviews.llvm.org/D138958 ___ cfe-commits mailing list cfe-commits

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. Last(?) ping of the year? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138958/new/ https://reviews.llvm.org/D138958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added a comment. (ping, maybe) In D138958#3968118 , @lebedev.ri wrote: > I've been thinking, and it seems to me that explicitly opting into > `__attribute__((nounwind))`-provided semantics should override > the `__attribute__((nothrow))`/`noe

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri marked an inline comment as done. lebedev.ri added a comment. I've been thinking, and it seems to me that explicitly opting into `__attribute__((nounwind))`-provided semantics should override the `__attribute__((nothrow))`/`noexcept` semantics. So looks like i should look into exception

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:2725 NoReturn = true; -if (FD->hasAttr()) +if (FD->hasAttr() || FD->hasAttr()) AddEHEdge = false; erichkeane wrote: > lebed

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added a comment. I'll take a look at rewording the docs if no one else does. I should hopefully have time next week, the rest of the patch is perhaps more important at the moment. Comment at: clang/docs/ReleaseNotes.rst:841 operator. -- ``clang_Cursor_getNumTem

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri updated this revision to Diff 479413. lebedev.ri marked 5 inline comments as done. lebedev.ri added a comment. @erichkeane thank you for taking a look! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138958/new/ https://reviews.llvm.org/D1

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri added inline comments. Comment at: clang/docs/ReleaseNotes.rst:841 operator. -- ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``, - ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and +- ``cl

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:841 operator. -- ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``, - ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and +- ``cl

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

lebedev.ri created this revision. lebedev.ri added reviewers: aaron.ballman, erichkeane, rjmccall. lebedev.ri added a project: LLVM. Herald added subscribers: carlosgalvezp, jdoerfert. Herald added a reviewer: NoQ. Herald added a reviewer: njames93. Herald added a project: All. lebedev.ri requested