[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT added a comment. Thanks for the review! I’ll need somebody to help land this, as I don’t have write access to the project. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143410/new/ https://reviews.llvm.org/D143410

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT added a comment. Test failures seem related to recent driver changes. There's a commit 6a8a423c1864ced060a7041bf6ada7574f35ad4d that purports to fix them. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT updated this revision to Diff 495314. DHowett-MSFT added a comment. - Use .copy() like the other nearby code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143410/new/ https://reviews.llvm.org/D143410 Files:

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT marked 2 inline comments as done. DHowett-MSFT added a comment. The new test checks how tightly things were packed across a stack of packs, pushed and popped. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143410/new/

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT updated this revision to Diff 495285. DHowett-MSFT added a comment. @mikerice Alright, it turns out that PragmaPackAlignStorage isn't suitable for this (actually, I'm not sure what it _is_ suitable for) because it's deleted by the time the template gets parsed again. The `push,

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT added a comment. While this fixes the assertion failure and the immediate issue of whether packing _works_ inside delay-parsed templates in a PCH, it does reveal a follow-on issue that I can't quite trace out. c++ template void foo() { #pragma pack(push, 1) #pragma

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

DHowett-MSFT created this revision. Herald added a project: All. DHowett-MSFT updated this revision to Diff 495195. DHowett-MSFT added a comment. DHowett-MSFT published this revision for review. Herald added a project: clang. Herald added a subscriber: cfe-commits. - Add a test DHowett-MSFT

[PATCH] D138453: [clang] Add serialization for loop hint annotation tokens

DHowett-MSFT added a comment. In D138453#4105209 , @shafik wrote: > It looks like https://github.com/llvm/llvm-project/issues/60543 is hitting > your added `llvm_unreachable("missing serialization code for annotation > token");` is this expected. I

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

DHowett-MSFT updated this revision to Diff 189193. DHowett-MSFT added a comment. Fixed corrupted patch. I know that the -str test case is not the _ideal_ location for the comdat test, but it is the only test whose invariants failed (the IR for the descriptor definition now contains `comdat,

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

DHowett-MSFT updated this revision to Diff 189183. DHowett-MSFT added a comment. This change caused a test to fail, so I took the opportunity to augment the test with COMDAT entry. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58807/new/

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188 + Twine ManglePublicSymbol(StringRef Name) { +return StringRef(CGM.getTriple().isOSBinFormatCOFF() ? "$_" : "._") + Name; DHowett-MSFT wrote: > As of the latest revision,

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188 + Twine ManglePublicSymbol(StringRef Name) { +return StringRef(CGM.getTriple().isOSBinFormatCOFF() ? "$_" : "._") + Name; As of the latest revision, this now fails at

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:1231 +break; + +auto Storage = llvm::GlobalValue::DefaultStorageClass; After we get the `ObjCInterfaceDecl`, we have to make sure it's the one corresponding to

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

DHowett-MSFT created this revision. DHowett-MSFT added a project: clang. Herald added a subscriber: cfe-commits. Without this change, linking multiple objects containing block descriptors together on Windows will generate duplicate symbol errors. Repository: rG LLVM Github Monorepo

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188 + StringRef SymbolPrefix() { +return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._"; theraven wrote: > DHowett-MSFT wrote: > > theraven wrote: > > > DHowett-MSFT wrote:

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188 + StringRef SymbolPrefix() { +return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._"; theraven wrote: > DHowett-MSFT wrote: > > Should this be `SymbolPrefix()` or

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

DHowett-MSFT added a comment. This looks great, and takes up the parts of my patch that I cared about. Thank you for doing this. My primary concern is that the patch includes my "early init" changes -- while I support it and think it's the right solution, especially where it reduces double

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

DHowett-MSFT added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1276 +InitVar->setSection(".CRT$XCLa"); +CGM.addUsedGlobal(InitVar); + } rjmccall wrote: > Is the priority system not good enough? My reading of the LLVM language reference leads

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

DHowett-MSFT added a subscriber: smeenai. DHowett-MSFT added inline comments. Comment at: include/clang/Driver/Options.td:1491 // Objective-C ABI options. -def fobjc_runtime_EQ : Joined<["-"], "fobjc-runtime=">, Group, Flags<[CC1Option]>, +def fobjc_runtime_EQ : Joined<["-"],

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

DHowett-MSFT accepted this revision. DHowett-MSFT added a comment. This revision is now accepted and ready to land. This looks good to me, but I don't have a strong understanding of "outlining." Comment at: lib/CodeGen/CGBlocks.cpp:1262 + if (IsWindows) { +auto *Init =

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

DHowett-MSFT added a comment. In https://reviews.llvm.org/D47233#1129207, @rjmccall wrote: > In https://reviews.llvm.org/D47233#1129110, @smeenai wrote: > > > WinObjC does this wrapping, for example. > > > I see. The idea of creating the type descriptors and mangled names ad-hoc > for the

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

DHowett-MSFT accepted this revision. DHowett-MSFT added a comment. This revision is now accepted and ready to land. This largely matches what we've done in the WinObjC clang patchset here

[PATCH] D46052: GNUstep Objective-C ABI version 2

DHowett-MSFT added inline comments. Comment at: lib/CodeGen/CGObjCGNU.cpp:439 + ArrayRef IvarOffsets, + ArrayRef IvarAlign, + ArrayRef IvarOwnership); While we're here, is there

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

DHowett-MSFT added a comment. Hi! Is there anything else holding up this patch? Thanks! Repository: rC Clang https://reviews.llvm.org/D45305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

DHowett-MSFT updated this revision to Diff 141701. DHowett-MSFT edited the summary of this revision. DHowett-MSFT added a comment. I've fixed the test to check the LLVM IR; I chose to use `CHECK-SAME` and use indentation to clarify what was going on. We're now checking that an empty protocol is

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

DHowett-MSFT added a comment. It seems more fragile to check the contents of the protocol rather than the invariant we care most about (its size). I'm willing to do that, and am updating the patch accordingly, but I don't want to commit to a more fragile test than is necessary. Illustratively,

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

DHowett-MSFT updated this revision to Diff 141450. DHowett-MSFT added a comment. Added a test per @rjmccall's suggestion. I chose to test this by emitting assembly because the llvm ir is significantly harder to pick apart for the true _size_ of the protocol. Repository: rC Clang

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT added a comment. Thanks! I don't have the means to check this in myself. Repository: rC Clang https://reviews.llvm.org/D44646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

DHowett-MSFT created this revision. DHowett-MSFT added reviewers: rjmccall, theraven. DHowett-MSFT added a project: clang. Herald added a subscriber: cfe-commits. Protocols that were being referenced but could not be fully realized were being emitted without `properties`/`optional_properties`.

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT updated this revision to Diff 141055. DHowett-MSFT added a comment. Switched from `i686-windows` to `i686-unknown-windows-msvc` as per @compnerd's suggestion. Based on the prior discussion around MSVC's definition of `va_{start,end,arg}` and how it is not a valid implementation

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT added a comment. Thank you for the review! I don't have the means to check this in myself. Repository: rC Clang https://reviews.llvm.org/D44580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT marked 5 inline comments as done. DHowett-MSFT added a comment. @compnerd CL warns for `__forceinline` variadics in C code as well: test.c(1): warning C4714: function 'void hello(int,...)' marked as __forceinline not inlined Repository: rC Clang

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT added inline comments. Comment at: test/Sema/ms-forceinline-on-variadic.cpp:14 +__builtin_va_end(ap); +} + efriedma wrote: > compnerd wrote: > > DHowett-MSFT wrote: > > > compnerd wrote: > > > > Would be nice to have a second test that uses the

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT added inline comments. Comment at: test/Sema/ms-forceinline-on-variadic.cpp:1 +// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify -fms-extensions %s \ +// RUN:| FileCheck %s compnerd wrote: > Personally, I prefer the fully

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT added a comment. In https://reviews.llvm.org/D44646#1042347, @efriedma wrote: > The compiler shouldn't inline functions which call va_start, whether or not > they're marked always_inline. That should work correctly, I think, at least > on trunk. (See

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT updated this revision to Diff 138996. DHowett-MSFT added a comment. Ran `clang-format` over changed lines. Reuploaded diff with full context. Repository: rC Clang https://reviews.llvm.org/D44580 Files: lib/Sema/SemaExpr.cpp test/SemaObjC/block-compare.mm Index:

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT updated this revision to Diff 138995. DHowett-MSFT added a comment. Fixed formatting. Repository: rC Clang https://reviews.llvm.org/D44646 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaDeclAttr.cpp test/Sema/ms-forceinline-on-variadic.cpp Index:

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT added a comment. Apologies if I've chosen the wrong set of reviewers. Repository: rC Clang https://reviews.llvm.org/D44646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

DHowett-MSFT created this revision. DHowett-MSFT added reviewers: rnk, majnemer. DHowett-MSFT added a project: clang. Herald added subscribers: cfe-commits, eraman. The MSVC compiler rejects `__forceinline` on variadic functions with the following warning (at /https://reviews.llvm.org/W4):

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT updated this revision to Diff 138868. DHowett-MSFT marked an inline comment as done. DHowett-MSFT added a comment. Backed out changes to block type assignment. Repository: rC Clang https://reviews.llvm.org/D44580 Files: lib/Sema/SemaExpr.cpp test/SemaObjC/block-compare.mm

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT planned changes to this revision. DHowett-MSFT marked an inline comment as done. DHowett-MSFT added inline comments. Comment at: lib/Sema/SemaExpr.cpp:7749 +// id (or strictly compatible object type) -> T^ +if (getLangOpts().ObjC1 &&

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT updated this revision to Diff 138759. DHowett-MSFT removed a subscriber: lebedev.ri. DHowett-MSFT added a comment. Moved files around slightly. Repository: rC Clang https://reviews.llvm.org/D44580 Files: lib/Sema/SemaExpr.cpp test/SemaObjC/block-compare.mm Index:

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT added a subscriber: lebedev.ri. DHowett-MSFT added a comment. In https://reviews.llvm.org/D44580#1040540, @lebedev.ri wrote: > Why not `test/SemaObjC/block_compare.mm` ? I wasn't are that it existed. It may even be a good fit for `test/SemaObjC/block-type-safety.m`, which already

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

DHowett-MSFT created this revision. DHowett-MSFT added reviewers: theraven, rjmccall. DHowett-MSFT added a project: clang. Herald added a subscriber: cfe-commits. This commit makes valid the following code: // objective-c++ #define nil ((id)nullptr) ... void (^f)() = ^{}; if (f == nil)