[PATCH] D128830: [Pipelines] Introduce DAE after ArgumentPromotion

dblaikie added a comment. In D128830#3745031 , @Michael137 wrote: > FYI, this broke the LLDB build bot: > https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46324/execution/node/74/log/ > > Looks like we're testing that inlined unused parameters

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added a comment. In D118511#3732200 , @tstellar wrote: > @dblaikie Can you file a bug and add the 15.0.0 Release Milestone, so we > don't forget to fix this in the release branch. Yeah, sorry about the delay on this, last week was a lot on my s

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie marked an inline comment as done. dblaikie added a comment. In D119051#3724121 , @rnk wrote: > Regarding the usage of isPOD in the MSVC ABI code, we should have a routine > similar to `isTrivialForAArch64MSVC` that works for the other MSVC > ar

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a comment. Each godbolt link shows MSVC 19.latest and clang, both in x86 and aarch64, with a reference pod and non-pod example at the start and end, and the variable test in the middle to see whether the codegen matches the pod or non-pod baselines above and below. > 1. checked

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. @MaskRay I think this change is probably the best point of comparison/demonstration of this patch direction (taking some minor liberties with this patch to simplify things somewhat in ways that have already been discussed/seem quite reasonable - specifically having `ge

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

dblaikie updated this revision to Diff 460637. dblaikie marked an inline comment as done. dblaikie added a comment. Add the three test cases. Verified they failed without the patch and pass with it. Testing them across all the Windows ABI variants, since they seem to apply equally. Repository:

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

dblaikie added a subscriber: majnemer. dblaikie added a comment. In D133817#3790057 , @rnk wrote: > Thanks! > >> But I had >> trouble figuring out how to exercise this functionality properly to add >> test coverage and then compare that to MSVC itself...

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

dblaikie added a comment. Which patch needs review, this one or D133875 ? Or are they both relevant? (their names are basically identical, so I'm not sure which is needed/how they're different) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D134339: [clang][llvm] generate accessibility metadata for type aliases

dblaikie added a comment. Looks pretty good to me - few things to clean up/simplify. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1331-1333 + if (isa(DC)) { +Flags = getAccessFlag(Ty->getDecl()->getAccess(), cast(DC)); + } https://llvm.org/docs/CodingSta

[PATCH] D134339: [clang][llvm] generate accessibility metadata for type aliases

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I'll commit this shortly. Comment at: llvm/lib/IR/DIBuilder.cpp:351 DIScope *Context, uint32_t AlignInBits, +

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:307 + /// decltype(s) will be printed as "S" if enabled and as "S<{1,2}>" if disabled, + /// regardless if PrintCanonicalTypes is enabled. + unsigned AlwaysIncludeTypeForNonTypeTemplateArgumen

[PATCH] D134339: [clang][llvm] generate accessibility metadata for type aliases

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG08288052aef2: [DebugInfo] Emit access specifiers for typedefs (authored by J-Camilleri, committed by dblaikie). Changed prior to commit: https://r

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a subscriber: aaron.ballman. dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:307 + /// decltype(s) will be printed as "S" if enabled and as "S<{1,2}>" if disabled, + /// regardless if PrintCanonicalTypes is enabled. + unsigne

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:307 + /// decltype(s) will be printed as "S" if enabled and as "S<{1,2}>" if disabled, + /// regardless if PrintCanonicalTypes is enabled. + unsigned AlwaysIncludeTypeForNonTypeTemplateArgumen

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:307 + /// decltype(s) will be printed as "S" if enabled and as "S<{1,2}>" if disabled, + /// regardless if PrintCanonicalTypes is enabled. + unsigned AlwaysIncludeTypeForNonTypeTemplateArgumen

[PATCH] D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers.

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:690-695 + // If we have an empty initializer then we do not want to create a guard var. + // 'Empty' needs only to apply to init functions that we call directly, calls + // to imported module initializ

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

dblaikie added subscribers: EricWF, rsmith. dblaikie added a comment. In D133425#3815118 , @aaron.ballman wrote: > In D133425#3780579 , @dblaikie > wrote: > >> > > Yeah, that might be a way forward - splitting

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:307 + /// decltype(s) will be printed as "S" if enabled and as "S<{1,2}>" if disabled, + /// regardless if PrintCanonicalTypes is enabled. + unsigned AlwaysIncludeTypeForNonTypeTemplateArgumen

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a subscriber: rsmith. dblaikie added a comment. > This also tries to fix the problem I raised a year ago: > https://discourse.llvm.org/t/make-command-line-support-for-c-20-module-uniform-with-gcc/59144 I think this thread is fairly different from what this patch is proposing. @rs

[PATCH] D134544: [clang-cl] Implement /ZH: flag

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:3220 Values<"simple,mangled">, Flags<[CC1Option, NoDriverOption]>; +def gsrc_hash_EQ : Joined<["-"], "gsrc-hash=">, + Group, Flags<[CC1Option, NoDriverOption]>, This is a dr

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

dblaikie created this revision. dblaikie added reviewers: hansw, rnk. Herald added subscribers: mstorsjo, kristof.beyls. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixes: Protected members, HFA: ht

[PATCH] D134705: [clang][DebugInfo] Emit debuginfo for non-constant case value

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. SGTM - could you also add some CHECKs in the test that show the enumeration was added to the enumerators list on the CU (& you can probably test with just one enumerator - don't need two i

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

dblaikie added a comment. Ping on this. (also sent out D134688 for the HFA stuff, which separates that from the "isTrivialFor" functionality entirely making it easier to make the different choices here) Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

dblaikie updated this revision to Diff 463379. dblaikie added a comment. Update comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134688/new/ https://reviews.llvm.org/D134688 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGen

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

dblaikie marked 2 inline comments as done. dblaikie added inline comments. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:4478-4480 // MSVC Windows on Arm64 considers a type not HFA if it is not an // aggregate according to the C++14 spec. This is not consistent with the

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a comment. Moving this to a top level discussion because it's easier to write replies here than in an inline comment. >> If it's that the tool is trying to get the type information purely from the >> string representation, maybe the tool should be doing things differently - >> r

[PATCH] D134825: [clang] Add debug info in MicrosoftCXXABI::EmitVirtualMemPtrThunk()

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers-debug-info.cpp:13 + +// CHECK: ![[DBG]] = !DILocation( + might be worth checking what location this is, to show it's a good one? (the code added makes some choice ab

[PATCH] D134825: [clang] Add debug info in MicrosoftCXXABI::EmitVirtualMemPtrThunk()

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good to me. (Ah, I see, that chrome bug actually hit the underlying problem, not even the verifier constraint - when the constraint is violated & not detected, then code gets inlin

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a comment. In D134453#3823141 , @DoDoENT wrote: >> Generally the way to do it, if you want to introspect into the type, its >> template parameters, etc, then yes, keeping pointers to the AST or reparsing >> the name with Clang as-needed,

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a comment. Again, I'm not advocating for the printing as-is, I think adding the top level name that disambiguates would be a good thing - and I think the GCC and MSVC examples somewhat show why adding all the other layers would be harmful to readability - th

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a comment. In D134453#3826832 , @aaron.ballman wrote: > In D134453#3825902 , @dblaikie > wrote: > >> Again, I'm not advocating for the printing as-is, I think adding the top >> level name

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

dblaikie added a comment. Ping on this Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134688/new/ https://reviews.llvm.org/D134688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llv

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

dblaikie added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133817/new/ https://reviews.llvm.org/D133817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

dblaikie added a comment. >> OK, so it sounds like the printing behavior change (not necessarily with a >> policy flag) necessary for diagnostics (make it non-ambiguous/not invalid >> code which is what is currently emitted) would be a good bug fix both for >> diagnostics and for ast-print? I

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

This revision was automatically updated to reflect the committed changes. Closed by commit rG165128989568: [clang] fix generation of .debug_aranges with LTO (authored by azat, committed by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1330

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

This revision was automatically updated to reflect the committed changes. Closed by commit rG2e1c1d6d7287: MSVC AArch64 ABI: Homogeneous aggregates (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134688/new/ https://reviews.llv

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4769976c49be: MSVC ABI: Looks like even non-aarch64 uses the MSVC/

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie updated this revision to Diff 465158. dblaikie added a comment. rebase and fix an AST test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/include/clang/Basic/LangOptions.h clang

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie updated this revision to Diff 465159. dblaikie added a comment. Remove Microsoft return ABI test, now that the Microsoft ABI implementation no longer depends on the AST isPOD property Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/ne

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a comment. In D119051#3747673 , @rnk wrote: > In D119051#3747201 , @dblaikie > wrote: > >> So... my conclusion is that Clang's AArch64 appears to be correct for x86 >> as well, and we should just

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie updated this revision to Diff 465161. dblaikie added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118511/new/ https://reviews.llvm.org/D118511 Files: clang/include/clang/Basic/DiagnosticASTKinds.td clang/include/cla

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036 // The align if the field is not packed. This is to check if the attribute // was unnecessary (-Wpacked). CharUnits UnpackedFieldAlign = !DefaultsToAIXPowerAlignment ? Fiel

[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

dblaikie added inline comments. Comment at: clang/lib/AST/Interp/Floating.h:27-29 + template struct Repr; + template <> struct Repr<32> { using Type = float; }; + template <> struct Repr<64> { using Type = double; }; jcranmer-intel wrote: > aaron.ballman wrot

[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

dblaikie added inline comments. Comment at: clang/lib/AST/Interp/Floating.h:27-29 + template struct Repr; + template <> struct Repr<32> { using Type = float; }; + template <> struct Repr<64> { using Type = double; }; tbaeder wrote: > dblaikie wrote: > > jcran

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie created this revision. dblaikie added a reviewer: rnk. Herald added subscribers: kristof.beyls, dschuff. Herald added a project: All. dblaikie requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a project: clang. Spinoff from D119051

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a comment. In D119051#3834985 , @rnk wrote: > Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it > makes sense to move the tail padding predicate from TargetCXXABI to > TargetInfo: > https://github.com/llvm/llvm-pr

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie updated this revision to Diff 465956. dblaikie added a comment. Move condition to TargetInfo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/include/clang/Basic/LangOptions.h cla

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:774-775 +if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) || +(getLangOpts().getClangABICompat() <= + LangOptions::ClangABI::Ver15 || Target.isPS() || Target.isOSD

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie updated this revision to Diff 465957. dblaikie added a comment. Fix to use CXXABI Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135326/new/ https://reviews.llvm.org/D135326 Files: clang/include/clang/Basic/TargetCXXABI.h clang/include

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added inline comments. Comment at: clang/lib/Basic/Targets/OSTargets.h:169 +if (T.getOS() == llvm::Triple::WatchOS || +this->getCXXABI().getKind() == TargetCXXABI::AppleARM64) + return TargetInfo::UseTailPaddingUnlessPOD11; rnk wrote: >

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added inline comments. Comment at: clang/lib/Basic/Targets/OSTargets.h:169 +if (T.getOS() == llvm::Triple::WatchOS || +this->getCXXABI().getKind() == TargetCXXABI::AppleARM64) + return TargetInfo::UseTailPaddingUnlessPOD11; dblaikie wrot

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3844605 , @ChuanqiXu wrote: > In D134267#3815453 , @dblaikie > wrote: > >>> This also tries to fix the problem I raised a year ago: >>> https://discourse.llvm.org/t/make-comm

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036 // The align if the field is not packed. This is to check if the attribute // was unnecessary (-Wpacked). CharUnits UnpackedFieldAlign = !DefaultsToAIXPowerAlignment ? Fiel

[PATCH] D119209: [clang] Add cc1 option -fctor-dtor-return-this

dblaikie added a comment. I was rather hoping we could have a more general "floating ABI" flag that we can lump in anything that we'd like to have otherwise but can't break ABI for - so users like Fuschia that know they're compiling everything with the same version of Clang can opt into the flo

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally `assert(0)` should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have a

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Can't seem to find an llvm-dev/commits discussion for r166821, but I remember discussing it several times before, perhaps this one happened on IRC and so we may not have any record of it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3847607 , @aaron.ballman wrote: > In D135551#3847444 , @dblaikie > wrote: > >> I thought this was settled quite a while ago and enshrined in the style >> guide: https://llvm

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. To the original point I was making about implicit modules (which might've been my own confusion due to the term being brought up in D134269 ): Implicit modules is the situation where the compiler, finding that it needs a module while

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Because I was looking, some other `assert(0)` -> `llvm_unreachable` cleanups (though, yes, even the earliest cleanups include some assert(0) -> report_fatal_error, but for externally/user-reachable failures, like invalid bitcode, I think). Some of these are more blanke

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3849673 , @iains wrote: > (trying not derail this discussion further) > > - Yes, the alternate solution proposed for the "Hello World" case would work > with the module mapper interface. However, the initial discussi

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. >> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? >> (that's the first thing any of us do is reproduce with an assertions build >> that may fail miles away from where a crash occurred because an invariant >> was violated much earlier) - th

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3850266 , @inclyc wrote: > This makes sense! However I think `assert(0)` should not be used in this > case, we could expose another `llvm_unreachable`-like api and probably > `llvm_report_error` shall be fine. Are th

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a subscriber: rnk. dblaikie added a comment. In D135551#3850391 , @aaron.ballman wrote: > In D135551#3850308 , @dblaikie > wrote: > >> In D135551#3850266

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie updated this revision to Diff 467024. dblaikie added a comment. Add doc comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/include/clang/Basic/LangOptions.h clang/include/cl

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a subscriber: rjmccall. dblaikie added a comment. (abandoned this, but still kind of curious) @rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3851672 , @dblaikie wrote: > (abandoned this, but still kind of curious) > > @rjmccall - any background/history of having the CXXABI distinct from the OS? > I guess the point might've been that the C ABI is part of/th

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3853324 , @rjmccall wrote: > In D135326#3851678 , @dblaikie > wrote: > >> In D135326#3851672 , @dblaikie >> wrote: >> >>> (abandoned

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:1569 + virtual bool areDefaultedSMFStillPOD(const LangOptions&) const; + rnk wrote: > Please add a doc comment for this, with some history about how previously > explicitly def

[PATCH] D135488: [codegen][WIP] Display stack layouts in console

dblaikie added a comment. In D135488#3854624 , @nickdesaulniers wrote: > Playing around with `llvm-dwarfdump` today, I think I found an interesting > example: > > Consider the following DWARF DIE: > > 0x00017e5e: DW_TAG_variable >

[PATCH] D135916: Itanium ABI: Pack non-pod members of packed types

dblaikie created this revision. dblaikie added reviewers: rnk, abrachet. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Seems there's a narrow case - where a packed type doesn't pack its base subobject

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

dblaikie added inline comments. Comment at: clang/docs/ReleaseNotes.rst:239-243 +- GCC doesn't pack non-POD members in packed structs unless the packed + attribute is also specified on the member. Clang historically did perform + such packing. Clang now matches the gcc behavior

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

dblaikie added a comment. In D117616#3732261 , @dblaikie wrote: > In D117616#3731188 , @abrachet > wrote: > >> While digging around failures we were seeing related to this I found some >> behavior that diverges

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a comment. Hmm - pinging this since my last comment didn't seem to send mail (at least not that appeared on llvm-commits archive) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 __

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3854141 , @rjmccall wrote: > I don't remember the history of the `-fc++-abi` flag at all, so if that's a > driver flag, you'll probably need to investigate it more to remove it. Maybe > it was added for testing purp

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. Herald added a subscriber: abrachet. Herald added a project: All. FWIW, the existence of this flag and the TargetCXXABI abstraction (which predates the flag, but might otherwise be more simply factored/implemented with inheritance rather than switch tables) came up in d

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration aaron.ballman wrote: > royjacobson wrote:

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3859235 , @leonardchan wrote: > In D85802#3858760 , @rjmccall wrote: > >> Does Fuchsia still need this? If those experiments have stabilized, maybe >> we can just remove it. Al

[PATCH] D135916: Itanium ABI: Pack non-pod members of packed types

This revision was automatically updated to reflect the committed changes. Closed by commit rG037f85668126: Itanium ABI: Pack non-pod members of packed types (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D135916?vs=467599&id=467890#toc Repository: rG LLVM Github Mo

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration anderslanglands wrote: > dblaikie wrote: >

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3859540 , @leonardchan wrote: >> Naive question, but what does it mean to target Fuschia but be >> gnu-compatible? (how would that be different than using whatever gnu OS >> (linux, etc) the other code was built for) >

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

dblaikie added inline comments. Comment at: clang/test/CodeGen/bounds-checking-fam.c:2 // REQUIRES: x86-registered-target -// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=0 -fsanitize=array-bounds%s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STR

[PATCH] D135713: [cmake][Fuchsia] Add -ftrivial-auto-var-init=zero to runtimes build

dblaikie added a comment. any chance of using pattern init rather than zero init, so this doesn't hide uninitialized bugs? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135713/new/ https://reviews.llvm.org/D135713

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

dblaikie added a comment. Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning? Repository: rG LLVM Github Monorepo

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. (sorry I've not been following all this as closely as I should - but I don't think BMIs are an implementation detail, anymore than object files are - users should/will be as aware of BMIs as they are of .o files - there build artifacts that need to be cached, can be ou

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3851479 , @ChuanqiXu wrote: > In D134267#3849629 , @dblaikie > wrote: > >> To the original point I was making about implicit modules (which might've >> been my own confusion

[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

dblaikie added a comment. In D139986#4003873 , @Mordante wrote: > In D139986#4001180 , @Michael137 > wrote: > >> Missed couple of test cases in libcxx >> About to fix those > > There were more breakage due to thi

[PATCH] D137838: [Support] Move TargetParsers to new component

dblaikie added a comment. This has introduced a circular dependency due to the forwarding header (forwarding header depends on new lib, new lib depends on support, where the forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the patch be reverted on that basis) though I

[PATCH] D140544: [DebugInfo] make DW_LANG_C11 respect -gstrict-dwarf

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:582 } else if (LO.C11) { -LangTag = llvm::dwarf::DW_LANG_C11; +if (CGO.DebugStrictDwarf && CGO.DwarfVersion < 5) + LangTag = llvm::dwarf::DW_LANG_C; Generally, I think,

[PATCH] D140544: [DebugInfo] make DW_LANG_C11 respect -gstrict-dwarf

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140544/new/ https://reviews.llvm.org/D140544 _

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

dblaikie added a comment. (pulling this out from an inline comment, because it's hard to quote/smaller than it needs to be for more complex discussions): > Oh, thanks for finding this. It is pretty bad that I didn't image we can > specify multiple input module units in one command line. > >> Wh

[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

dblaikie added a comment. In D139986#4008169 , @Mordante wrote: > In D139986#4005997 , @dblaikie > wrote: > >> In D139986#4003873 , @Mordante >> wrote: >> >>> In D139986

[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

dblaikie added a comment. Oh, and I meant to mention https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a doesn't seem, at least to my cursory reading, to show diagnostic checking failures, but crashes? Is there something broken in the UI that's meant t

[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

dblaikie added a comment. In D139986#4017650 , @Mordante wrote: > In D139986#4017467 , @dblaikie > wrote: > >> In D139986#4008169 , @Mordante >> wrote: >> >>> In D139986

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

dblaikie added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:52 + + enum class TriState : int { kYes, kNo, kUnknown }; + We don't usually use the `k` prefix for enums (the style guide mentions using an acronym like `TS_` - though even tha

[PATCH] D140860: [Diagnostics][NFC] Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values

dblaikie added a comment. > This behavior can lead to the lack of warnings for complicated constexpr > expressions. Are those bugs, though? If the parentheses wouldn't change the outcome, due to the RHS being constant, maybe these undiagnosed cases aren't reflective of source code bugs/coding

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. My objections to this initially still seem to stand - the discourse thread doesn't appear to have shown more consensus & I still don't think we should remove a top level naming convention entirely. We should have a convention documented and specified in the top level f

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. (at least if/when D140585 sticks, then the format should be reflected at the top level, not omitted/left to subprojects to specify - then/at the same time, the explicit naming convention specified in mlir, compiler-rt, etc, should be

[PATCH] D140860: [Diagnostics][NFC] Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values

dblaikie added a comment. In D140860#4028089 , @hazohelet wrote: > As you point out, enhancement may be more accurate than bug fix. > There are rare cases where enabling a warning for missing parentheses in > `constexpr` logical expressions can be helpfu

[PATCH] D140860: [Diagnostics][NFC] Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values

dblaikie added a comment. The risk now is that this might significantly regress/add new findings for this warning that may not be sufficiently bug-finding to be worth immediate cleanup, causing users to have to choose between extensive lower-value cleanup and disabling the warning entirely. Ha

[PATCH] D140860: [Diagnostics][NFC] Fix -Wlogical-op-parentheses warning inconsistency for const and constexpr values

dblaikie added a comment. In D140860#4033530 , @hazohelet wrote: > I have yet to do thorough checks using this patched clang to build > significant code bases. > It will likely take quite a bit of time as I am not used to editing build > tool files. FW

<    5   6   7   8   9   10   11   12   13   14   >