[PATCH] D150340: [SEH]:Fix assertion when try is used inside catch(...) block with /EHa

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM... but I don't think the IR we're generating is really correct overall; see https://github.com/llvm/llvm-project/issues/62723 On a side-note, other open issues related to -EHa/__try:

[PATCH] D150340: [SEH]:Fix assertion when try is used inside catch(...) block with /EHa

efriedma added a comment. > I'd really want to look at IR for return inside try/finally, but for some > reason, I an not build compiler with your patch: no member named > 'setHasAddressTaken' in 'llvm::MachineBasicBlock'; did you mean > 'hasAddressTaken'. I may missing some thing in my environm

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGException.cpp:1833 +/// Find all local variable captures in the statement. +struct ReturnStmtFinder : ConstStmtVisitor { + bool ContainsRetStmt = false; rnk wrote: > We have the option to generalize

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma updated this revision to Diff 522773. efriedma added a comment. Herald added a subscriber: arichardson. Rebased so it builds, and added a couple tests, to unblock anyone wanting to look at this further. Haven't re-done my runtime testing. Still haven't addressed all the review comment

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma updated this revision to Diff 522782. efriedma added a comment. Fixup a couple LLVM tests which are failing; I think they're affected by the WinException.cpp changes? Maybe need to look a bit more closely to see if the changes make sense. Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. > This is an adaptation of the IBM XL compiler's -qstatsym option, which is > meant to generate symbol table entries for static variables. An artifact of > that compiler is that static variables are often not discarded even when > unused. Oh, I see; the compiler actua

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

efriedma added a comment. Is there some reason we actually need to do this whole dance in C++? The whole point of "inline builtins" was to handle constructs in the glibc headers that involve implementations of libc functions that somehow end up recursively calling themselves instead of a real

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

efriedma added a comment. At first glance, this seems like the wrong place to put this cast. If an expression in the AST produces a pointer with generic pointer type, then CodeGen should generate code for that expression that returns a generic pointer type. We shouldn't wait until the pointer

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

efriedma added a comment. > Also note, one of the bugs I reference shows how this breaks SFINAE: > https://github.com/llvm/llvm-project/issues/57176 and that is not easily > fixable. So this is non-conforming since it breaks valid code. You can mark a warning diagnostic SFINAEFailure to ensure

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

efriedma added a comment. In D150226#4354063 , @efriedma wrote: >> Also note, one of the bugs I reference shows how this breaks SFINAE: >> https://github.com/llvm/llvm-project/issues/57176 and that is not easily >> fixable. So this is non-conforming sin

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

efriedma added a comment. I guess to make that work, the constant evaluator would need to track whether we're in an SFINAE context (Sema::isSFINAEContext()). Based on that, we'd need to explicitly generate an error if we're in an SFINAE context, and a warning if we're not in such a context. C

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

efriedma added a comment. This seems like a weird way to fix this. The point of an "inline builtin" is that the inline function is actually the original function; it's just the inline implementation is only used in limited circumstances (in particular, it can't be used recursively). Changing

[PATCH] D148723: [clang] Enforce internal linkage for inline builtin

efriedma added a comment. In D148723#4283094 , @serge-sans-paille wrote: > In D148723#4280916 , @efriedma > wrote: > >> The point of an "inline builtin" is that the inline function is actually the >> original f

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma added a comment. I'm having trouble understanding the changes on the clang side. If I'm following correctly; the "denormal-fp-math" setting is a promise from the user to the compiler: if the setting is not "dynamic", the user promises that the definition will only execute in the specif

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma added a comment. If you have a library function that's built with "denormal-fp-math"="dynamic,dynamic", you can link it into code built in any mode, and LTO should be able to propagate that mode from the caller to the callee. That doesn't require clang to do anything special; you can

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Oh, sorry, I missed that the new code specifically runs on functions imported using -mlink-builtin-bitcode. I somehow thought it was running on all functions. LGTM CHANGES SINCE LAST A

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

efriedma added a comment. I'm a little concerned that this will explode in unexpected ways... in particular, it'll fail to link if the function doesn't actually exist externally. Which it probably doesn't if it would otherwise be linkonce_odr. CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

efriedma added a comment. Really, I'd prefer to keep isInlineBuiltinDeclaration() targeted as narrowly as possible; part of that is making it not trigger for C++ inline functions (which it never did in the past). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148723/new/ https://review

[PATCH] D148091: [clang][CodeGen] Break up TargetInfo.cpp [3/6]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148091/new/ https://reviews.llvm.org/D148091 ___

[PATCH] D148090: [clang][CodeGen] Break up TargetInfo.cpp [2/6]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148090/new/ https://reviews.llvm.org/D148090 ___

[PATCH] D148089: [clang][CodeGen] Break up TargetInfo.cpp [1/6]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148089/new/ https://reviews.llvm.org/D148089 ___

[PATCH] D148092: [clang][CodeGen] Break up TargetInfo.cpp [4/6]

efriedma added a comment. Is this supposed to be different from D148093 ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148092/new/ https://reviews.llvm.org/D148092 ___

[PATCH] D148094: [DRAFT][clang][CodeGen] Break up TargetInfo.cpp [6/6]

efriedma added a comment. Restructuring seems overall reasonable. (I'm assuming there isn't actually any changed code, just moving code around.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148094/new/ https://reviews.llvm.org/D148094 _

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:1250 + for (unsigned I = 1, IE = ID->getChainingSize(); I < IE; ++I) +mangleSourceName(""); + Weird indentation Comment at: clang/lib/AST/MicrosoftMangle

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Herald added subscribers: bd1976llvm, jplehr. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520 + getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit)) +return; + efriedma wrote: > I don't really like the

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. (I apologize it took me so long to get back to reviewing this.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:1250 + for (unsigned I = 1, IE = ID->getChainingSize(); I < IE; ++I) +mangleSourceName(""); + bolshakov-a wrote: > efriedma wrote: > > Weird indentation > I agree. Don't kn

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:1250 + for (unsigned I = 1, IE = ID->getChainingSize(); I < IE; ++I) +mangleSourceName(""); + bolshakov-a wrote: > efriedma wrote: > > bolshakov-a wrote: > > > efriedma wro

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:1250 + for (unsigned I = 1, IE = ID->getChainingSize(); I < IE; ++I) +mangleSourceName(""); + bolshakov-a wrote: > bolshakov-a wrote: > > efriedma wrote: > > > bolshakov-a

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM (Do you have commit access? If not, please specify the name/email you want for the "author" field.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146386/new/ https://revie

[PATCH] D147266: [AArch64] Add IR intrinsics for vbsl* C intrinsics

efriedma added a reviewer: dmgreen. efriedma added subscribers: bsmith, paulwalker-arm, lenary. efriedma added a comment. The primary tradeoff here is that existing optimizations won't understand the intrinsic... for example, we can't constant-fold, or automatically invert the mask. But making

[PATCH] D147266: [AArch64] Add IR intrinsics for vbsl* C intrinsics

efriedma added a comment. I guess I should note both the examples in https://github.com/llvm/llvm-project/issues/49305 could probably be fixed in other ways... we have heuristics to, for example, sink logic ops into loops when it's profitable. But that requires someone to notice the specific i

[PATCH] D148800: [C2x] Update 'nullptr' implementation based on CD comments

efriedma added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:10087 +// a conversion. +Kind = CK_NoOp; +return Compatible; I'd like to see testcases for: - Codegen (LLVM IR emission) - Constant evaluation. (Is a cast like this allowed in a

[PATCH] D147626: [clang] Reject flexible array member in a union in C++

efriedma added a comment. > If there's not indications of this being disruptive on non-MSVC-compatible > targets, then we may still be able to get away with rejecting the extension > there. If we need to have the codepath anyway, there isn't much harm in allowing it on all targets, I think. T

[PATCH] D148800: [C2x] Update 'nullptr' implementation based on CD comments

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148800/new/ https://reviews.llvm.org/D148800 ___ cfe-commits mailing list cfe-commi

[PATCH] D147626: [clang] Reject flexible array member in a union in C++

efriedma added a comment. In D147626#4316212 , @aaron.ballman wrote: > In D147626#4316190 , @efriedma > wrote: > >>> If there's not indications of this being disruptive on non-MSVC-compatible >>> targets, then

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcd93532dfc45: [MS ABI] Fix C++ mangling references to declarations. (authored by bolshakov-a, committed by efriedma). Repository: rG LLVM Github M

[PATCH] D148093: [clang][CodeGen] Break up TargetInfo.cpp [5/6]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148093/new/ https://reviews.llvm.org/D148093 ___

[PATCH] D148092: [clang][CodeGen] Break up TargetInfo.cpp [4/6]

efriedma added a comment. Is there some alternative way we can write this? Even if each of the overrides is only technically used in one place, it's a repeating pattern, and writing the casts inline makes it really hard to read. (Maybe the helpers can be somewhere else?) Repository: rG LL

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. Possibly the alloca doesn't get eliminated, or doesn't get eliminated early enough for attributor to kick in. Not sure what context would make that significant, off the top of my head. Is the lambda in question defined in an inline function (linkonce_odr)? Maybe we

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. > EmitLambdaDelegatingInvokeBody emit an always_inline hint To clarify, I mean on the call instruction, not on either function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132275/new/ https://reviews.llvm.org/D132275 _

[PATCH] D150215: [clang][CodeGen] Break up TargetInfo.cpp [7/8]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150215/new/ https://reviews.llvm.org/D150215 ___

[PATCH] D148094: [clang][CodeGen] Break up TargetInfo.cpp [8/8]

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148094/new/ https://reviews.llvm.org/D148094 ___

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added a comment. Two points I'm not sure about in the current version: - Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; `E->isGLValue()` is always true

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added a comment. The following also crashes, with no MaterializeTemporaryExpr involved. struct X { short n; char c; }; struct Y { _Atomic(X) a; _Atomic(int) b; }; constexpr X x{}; int z; Y y = { x, z }; Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added a comment. > Yeah, but not because of this patch; that's a pre-existing issue. Right; the _Atomic crashes are a pre-existing issue unrelated to MaterializeTemporaryExpr, so you shouldn't be trying to solve it by messing with HasAnyMaterializeTemporaryExpr. Repository: rG LLVM

[PATCH] D73459: [ARM] Add documentation for -march= and -mfpu= command line options

efriedma reopened this revision. efriedma added a comment. This revision is now accepted and ready to land. Herald added a project: All. This patch was reverted by 9624beb38a46037f69362650a52e06d8be4fd006 . I have a patch pla

[PATCH] D152396: [clang][doc] Rescue some deleted bits of the command-line reference.

efriedma created this revision. efriedma added reviewers: serge-sans-paille, aaron.ballman. Herald added a subscriber: dschuff. Herald added a project: All. efriedma requested review of this revision. Herald added a subscriber: aheejin. Herald added a project: clang. Back when the command-line ref

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. > The first argument in the call is an undef but the argument type is also > marked as noundef, so this is unreachable It looks like your code was getting miscompiled? If I'm understanding correctly, without this patch, we assume the lambda body is unreachable, and s

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + This needs a comment explaining why we're bailing out here. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:166

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + efriedma wrote: > This needs a comment explaining why we're bailing out here. We might need to do a recursive visit still, to handl

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + nickdesaulniers wrote: > efriedma wrote: > > efriedma wrote: > > > This needs a comment explaining why we're bailing out here. > >

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:3097 + FD->getLocation(), FD->getLocation()); +CGF.EmitFunctionBody(FD->getBody()); +CGF.FinishFunction(); Is there any way we can use GenerateCode as the entry

[PATCH] D152275: Use memory region declaration intrinsic when generating code for array subscripts

efriedma added reviewers: aaron.ballman, courbet, nikic, jdoerfert, nlopes, jeroen.dobbelaere. efriedma added a comment. If we are going to do this at all, I think this is roughly what it should look like. Potential issues you might run into: - The compile-time overhead of creating a bunch of

[PATCH] D152396: [clang][doc] Rescue some deleted bits of the command-line reference.

This revision was automatically updated to reflect the committed changes. Closed by commit rGf51924d124bd: [clang docs] Rescue some deleted bits of the command-line reference. (authored by efriedma). Changed prior to commit: https://reviews.llvm.org/D152396?vs=529422&id=531448#toc Repository:

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1472 +// the call operator body. +EmitLambdaStaticInvokeBody(cast(FD)); } else if (FD->isDefaulted() && isa(FD) && akhuang wrote: > efriedma wrote: > > Does this pass the

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:917 + bool VisitArraySubscriptExpr(const ArraySubscriptExpr *A) { +return Visit(A->getBase()) || Visit(A->getIdx()); + } Should only need to visit base, not idx. Repository:

[PATCH] D152275: Use memory region declaration intrinsic when generating code for array subscripts

efriedma added a comment. In D152275#4423845 , @simeon wrote: >> - User code might not actually obey the language rules; do we have any >> sanitizer that checks if user code trips over this? > > I believe AddressSanitizer >

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1470 +->getLambdaStaticInvoker()) && + !Fn->getName().contains("__impl")) { +// If emitting a lambda with static invoker on X86 Windows, change --

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma created this revision. efriedma added reviewers: rnk, hans, dblaikie, sigatrev, samtebbs, rjmccall. Herald added subscribers: mstorsjo, kristof.beyls. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. MSVC normally has a bunch of restr

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137872/new/ https://reviews.llvm.org/D137872 ___

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma updated this revision to Diff 532334. efriedma added a comment. Restrict to AArch64. Actually, it seems like something sort of similar happens with x86 vectorcall. But I haven't tried to test all the permutations of that, so don't modify the behavior for now. Repository: rG LLVM G

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

efriedma added inline comments. Comment at: clang/test/CodeGen/windows-seh-EHa-CppCondiTemps.cpp:3 +// FIXME: this check appears to be miscompiled? +// XFAIL: * lebedev.ri wrote: > tentzen wrote: > > lebedev.ri wrote: > > > This test broke once we always starte

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. (-Wglobal-constructors warning is still not implemented.) Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderG

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:572 PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn)); + } else if (D->hasConstantInitialization() && !(D->hasAttr())) { +OrderGlobalInitsOrStermFinalizers Key(201,

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

efriedma added inline comments. Comment at: clang/include/clang/AST/Decl.h:3063 + /// Determine if this field is a potentially-overlapping, that is, + /// subobject with the [[no_unique_address]] attribute + bool isPotentiallyOverlapping() const; Maybe clarify

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139741/new/ https://reviews.llvm.org/D139741 ___

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. Not sure exactly what code that generates in its current form, but that's roughly the right idea, yes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 ___ cfe-commits maili

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

efriedma added a comment. Does the following work with the updated patch? class Empty {}; class UnionClass : Empty { [[no_unique_address]] union X { private: Empty x; alignas(2) char C; } U; char C; }; UnionClass L; Repository: rG LLVM Github Monorepo CHAN

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. Right, the call to atexit from the constructor function shouldn't be there. I think you need to add a "skip emitting the destructor" flag to EmitCXXCtorInit CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/new/ https://reviews.llvm.org/D137107 __

[PATCH] D27800: Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo

efriedma requested changes to this revision. efriedma added a comment. This revision now requires changes to proceed. Missing regression test in test/SemaCXX/. https://reviews.llvm.org/D27800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D28526: [ARM] Add diagnostics when initialization global variables with ropi/rwpi

efriedma requested changes to this revision. efriedma added a reviewer: efriedma. efriedma added a comment. This revision now requires changes to proceed. We really should be checking this much earlier. IsGlobalLValue() in ExprConstant.cpp is the canonical place to answer the question "is the ad

[PATCH] D28526: [ARM] Add diagnostics when initialization global variables with ropi/rwpi

efriedma added a comment. If there isn't already a LangOptions flag which reflects this, it probably makes sense to add one. https://reviews.llvm.org/D28526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D27800: Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo

efriedma added inline comments. Comment at: test/SemaCXX/pr31042.cpp:1 +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -disable-free %s + Oh, this testcase doesn't actually crash on trunk without at least -emit-llvm because semantic analysis

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

efriedma added a comment. I'm not sure we actually want to skip these checks for DeclRefExps. I mean, you can rely on the backend to correctly align a local variable (assuming the stack is correctly aligned), but it's very easy to misalign a global. https://reviews.llvm.org/D30283

[PATCH] D27800: Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo

efriedma added inline comments. Comment at: test/SemaCXX/pr31042.cpp:1 +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -disable-free %s + You need to use "-o -" or something like that to avoid generating a file pr31042.ll. Also, a comment explai

[PATCH] D30009: Add support for '#pragma clang attribute'

efriedma added a comment. Looking over the most recent version, I'm happy with the general semantics of push with apply_only_to. I'm not sure I see the point of apply_to: it doesn't allow the user to do anything that can't be done with apply_only_to. Also, if the apply_to list for an attribut

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

efriedma added a comment. It's possible to misalign a global definition by misaligning its section with a linker script... but we can probably ignore that possibility. It's very easy to misalign global declaration, though; for example: a.c: extern int a[]; int f(int x) { return a[x]; } b.

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

efriedma added inline comments. Comment at: test/CodeGenCXX/ubsan-global-alignment.cpp:9 +extern S1 S1_array[]; +extern S1 *S1ptr_array[]; + Probably a good idea to also test extern globals which aren't arrays; arrays are sort of a special-case due to array->poi

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

efriedma added a comment. Oh, right... constant folding uses the declared alignment of the global to constant-fold the comparison. (I still think it would be interesting to solve, but maybe orthogonal to some extent.) https://reviews.llvm.org/D30283

[PATCH] D31885: Remove TBAA information from LValues representing union members

efriedma added a comment. In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > I'm not sure this is the right way to do this; I suspect we're lumping > together a bunch of different bugs: > > 1. vector types need to have tbaa which makes them alias with their element > types [to be clea

[PATCH] D30283: [ubsan] Reduce alignment checking of C++ object pointers

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. Comment at: test/CodeGenCXX/ubsan-suppress-checks.cpp:25 +// LAMBDA: and i64 %[[THISINT2]], 3, !nosanitize +// LAMBDA: call void @__ubsan_handle_type_mismat

[PATCH] D31885: Remove TBAA information from LValues representing union members

efriedma added a comment. > Such an effective type change must be more explicit than "i allocated > typeless memory, and so i can do what i want with it". How can you change the effective type of malloc'ed memory in C, if storing a value of a new type doesn't have any effect? memset? A new C

[PATCH] D28820: Warn when calling a non interrupt function from an interrupt on ARM

efriedma added a comment. Why can't the compiler handle this case itself transparently? According to your description, the interrupt calling convention is different from the normal hard-float AAPCS calling convention: the VFP registers are all callee-save. The compiler knows this; it should b

[PATCH] D28820: Warn when calling a non interrupt function from an interrupt on ARM

efriedma added a comment. > There would be a big performance penalty for ISRs with callees that don't use > VFP regs. Sacrificing correctness for the sake of performance seems like a bad idea... especially given that the optimizer can insert calls to memcpy where they didn't originally exist i

[PATCH] D27800: Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo

efriedma added inline comments. Comment at: lib/Sema/SemaExpr.cpp:4031 // C99 6.5.3.4p4: the type (an unsigned integer type) is size_t. + if (isUnevaluatedContext() && ExprKind == UETT_SizeOf && + TInfo->getType()->isVariablyModifiedType()) Is the isUnev

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

efriedma added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:72 + if (const auto *UO = dyn_cast(Op.E)) +return IsPromotedInteger(UO->getSubExpr()); + Checking isPromotableIntegerType doesn't work the way you want it to; types can be "promoted" w

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

efriedma added a comment. I think you're running into https://llvm.org/bugs/show_bug.cgi?id=31620 . https://reviews.llvm.org/D29542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29736: [WebAssembly] Add target specific overrides for lgamma family functions

efriedma added a comment. It doesn't make sense to do this in WebAssembly-specific code; every POSIX platform has a signgam which behaves the same way. https://reviews.llvm.org/D29736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D34523: AST: mangle BlockDecls under MS ABI

efriedma added inline comments. Comment at: lib/AST/MicrosoftMangle.cpp:988 + if (const auto *RD = dyn_cast(DC)) +mangleName(RD); + else The call to mangleName() looks a little weird... I would have expected a call to mangleUnqualifiedName or s

[PATCH] D34523: AST: mangle BlockDecls under MS ABI

efriedma added inline comments. Comment at: test/CodeGenCXX/msabi-blocks.cpp:90 +} + compnerd wrote: > efriedma wrote: > > The Itanium ABI document lists five cases where the mangling is externally > > visible. I think this is missing a testcase for the "initia

[PATCH] D34523: AST: mangle BlockDecls under MS ABI

efriedma added a comment. LGTM (with the caveat that I don't know anything about Microsoft mangling). https://reviews.llvm.org/D34523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34590: [ubsan] Diagnose invalid uses of builtins (clang)

efriedma resigned from this revision. efriedma added a comment. The check itself looks okay, but I'm not really familiar with the other ubsan bits. https://reviews.llvm.org/D34590 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D34568: [Sema] Make BreakContinueFinder handle nested loops.

efriedma added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D34568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

<    12   13   14   15   16   17   18   >