[PATCH] D140224: [Driver] Revert D139717 and add -Xparser instead

davide added a comment. In D140224#4014256 , @MaskRay wrote: > In D140224#4014243 , @rsundahl > wrote: > >>> I'll reject [-\Xparser for a while as well. This is a valid amendment to >>> D139717

[PATCH] D140224: [Driver] Revert D139717 and add -Xparser instead

davide added a comment. In D140224#4014245 , @MaskRay wrote: > In D140224#4014234 , @davide wrote: > >> In D140224#4014230 , @MaskRay >> wrote: >> >>> In D140224#4014203

[PATCH] D140224: [Driver] Revert D139717 and add -Xparser instead

davide added a comment. In D140224#4014230 , @MaskRay wrote: > In D140224#4014203 , @davide wrote: > >> @MaskRay Roy hasn't replied. We found other spellings that break (e.g. >> `-Xcctests` or something). Revert

[PATCH] D140224: [Driver] Revert D139717 and add -Xparser instead

davide reopened this revision. davide added a comment. This revision is now accepted and ready to land. @MaskRay Roy hasn't replied. We found other spellings that break (e.g. `-Xcctests` or something). Revert this patch until we find an agreement. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D140224: [Driver] Revert D139717 and add -Xparser instead

davide added a comment. Fine if Roy is happy. If we find another thing that break, we'll let you know. Please wait for him to comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140224/new/ https://reviews.llvm.org/D140224

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide added a comment. In D139717#4001702 , @MaskRay wrote: > In D139717#4001688 , @davide wrote: > >> In D139717#4001685 , @MaskRay >> wrote: >> >>> In D139717#3998077

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide added a comment. In D139717#4001685 , @MaskRay wrote: > In D139717#3998077 , @manojgupta > wrote: > >> Xlinker still works. Xcompiler is failing. >> >> A google search will show that Xcompiler is a

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide accepted this revision. davide 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/D139717/new/ https://reviews.llvm.org/D139717

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide added a comment. In D139717#3997193 , @MaskRay wrote: > In D139717#3991765 , @rsundahl > wrote: > >> In D139717#3987963 , @MaskRay >> wrote: >> This change

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide added a comment. In D139717#3987963 , @MaskRay wrote: >> This change is breaking internal builds. We use the -Xfoo pattern but can >> now no longer manage whether we allow an unused -Xfoo option to pass as a >> warning or promote it to an error.

[PATCH] D139717: Revert "[Driver] Remove Joined -X"

davide added a comment. In D139717#3984648 , @lebedev.ri wrote: > This is missing a test, like the original commit mentioned. > Why can you not just use the the split variant, `-X clang ...`? This breaks many projects internally. There's no real

[PATCH] D138947: [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr

davide added a comment. LGTM, again. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138947/new/ https://reviews.llvm.org/D138947 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D138947: [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr

davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. LGTM with the comment reworded. Thanks. Comment at: clang/lib/Sema/SemaExprCXX.cpp:1463 + assert((!ListInitialization || Exprs.size() == 1) && + "Too many

[PATCH] D138947: [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr

davide added a comment. `Exprs.size() == 1` is still valid in every example we've seen, so yes, you might want to keep it (and update the assertion message) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138947/new/ https://reviews.llvm.org/D138947

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present for all archs

davide accepted this revision. davide 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/D122258/new/ https://reviews.llvm.org/D122258

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present for all archs

davide added a comment. In D122258#3479045 , @int3 wrote: > Thanks for the feedback! > >> The binary compatibility issue is non-existent on arm64, but it could be >> still a thing on x86_64. > > Can we enable `OmitDwarfIfHaveCompactUnwind` by default on

[PATCH] D122258: [MC] Omit DWARF unwind info if compact unwind is present for all archs

davide requested changes to this revision. davide added a comment. This revision now requires changes to proceed. Compact unwind was developed as a goal to replace the DWARF unwind. Some apps did not work if the OS was missing DWARF unwind, so we kept both for Intel. The binary compatibility

[PATCH] D113456: Allow protocol metadata to be deduplicated within dylibs

davide abandoned this revision. davide added a comment. Herald added a project: All. We fixed this in the linker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113456/new/ https://reviews.llvm.org/D113456

[PATCH] D113456: Allow protocol metadata to be deduplicated within dylibs

davide created this revision. davide added reviewers: rjmccall, rsmith, pete, ab, jckarter. davide requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Given these two files: #import @interface Bar : NSObject @end

[PATCH] D98458: Revert "[CodeGenModule] Set dso_local for Mach-O GlobalValue"

davide added a comment. + Lang, who probably has a better understanding of this, for visibility. I don't think this should be blocked further, but he might have a chance to comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98458/new/

[PATCH] D98458: Revert "[CodeGenModule] Set dso_local for Mach-O GlobalValue"

davide accepted this revision. davide 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/D98458/new/ https://reviews.llvm.org/D98458 ___

[PATCH] D98458: Revert "[CodeGenModule] Set dso_local for Mach-O GlobalValue"

davide added a comment. In D98458#2620731 , @MaskRay wrote: >> Mach-O doesn't support dso_local and this change broke XNU because of the >> use of dso_local. > > Can you elaborate on the unsupportness? For example, the backend can ignore > dso_local if

[PATCH] D76100: Debug Info: Store the SDK in the DICompileUnit.

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

[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

davide added a comment. [and Raphael for the clang vendor bits] Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65534/new/ https://reviews.llvm.org/D65534 ___ cfe-commits mailing list

[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

davide added a comment. Really on the lldb side, Jonas is the right person to review this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65534/new/ https://reviews.llvm.org/D65534 ___

[PATCH] D65116: [Driver] Set the default win32-macho debug format to DWARF

davide accepted this revision. davide added a comment. Minor otherwise LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65116/new/ https://reviews.llvm.org/D65116 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D58930: Add XCOFF triple object format type for AIX

davide requested changes to this revision. davide added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079 + if (log) +log->Printf("sorry: unimplemented

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

davide accepted this revision. davide added a comment. LGTM, sorry. for the delay. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55085/new/ https://reviews.llvm.org/D55085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

davide added a comment. In D53818#1312133 , @martong wrote: > > I reverted this change because it breaks a bunch of lldb tests (on MacOS, > > and probably on other platforms too). To be clear, part of the reason I'm > > reacting strongly here is that

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

davide added a comment. I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

davide added a comment. This doesn't break anything, and I'm fairly confident Raphael (@teemperor) ran the tests. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54898/new/ https://reviews.llvm.org/D54898 ___

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. Alexsei, I'm afraid I'm not qualified to review this patch. I would really recommend you to find somebody who's familiar with clang to review it, as it already seems to have broken lldb in the past. Repository: rC Clang https://reviews.llvm.org/D44100

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. In https://reviews.llvm.org/D44100#1282157, @a.sidorin wrote: > Hello everyone. > @martong : this version of patch reorders only fields and friends. No method > reordering is done (I can check if it works, though, and add the feature). > @davide: Unfortunately, I was

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. No worries :) l In https://reviews.llvm.org/D44100#1282148, @martong wrote: > Hi Davide, > > I'd like to draw your attention to an important relation of this patch to a > bug in LLDB which manifests as incorrect vtable layouts for LLDB expression > code. > Please see

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added subscribers: shafik, davide. davide added a comment. This patch broke lldb, at least on MacOS. You can reproduce the issue building lldb, applying your patch and then running $ ./lldb-dotest -p TestCModules I'm going to revert this to unblock folks, but don't hesitate to ping me

[PATCH] D53807: Create a diagnostic group for warn_call_to_pure_virtual_member_function_from_ctor_dtor, so it can be turned into an error using Werror

davide added a comment. While it's true that I implemented this warning, I don't feel qualified enough to approve the introduction of a new flag to clang. Probably @rsmith should sign this off. Repository: rC Clang https://reviews.llvm.org/D53807

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

davide added a comment. This broke all the lldb bots as well, FWIW. https://reviews.llvm.org/D44263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

davide reopened this revision. davide added a comment. This revision is now accepted and ready to land. The lldb bot started failing very recently and the blamelist hints at this change. http://green.lab.llvm.org/green/job/lldb-cmake// Can you please take a look? For your convenience,

[PATCH] D44069: Test Driver sanitise, unsupported on OpenBSD

davide added a comment. Apologies, but this is not an area I'm particularly familiar with. So, I'm resigning, but @filcab / @vsk can probably comment. https://reviews.llvm.org/D44069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43423: [SimplifyCFG] Create flag to disable simplifyCFG.

davide added subscribers: chandlerc, davide. davide added a comment. Some high level comments: 1. This is something that GCC does relatively frequently (adding frontend options to control optimization passes), but LLVM tends to not expose these details. FWIW, I'd very much prefer the details

[PATCH] D34082: [Frontend] 'Show hotness' can be used with a sampling profile

davide added a comment. In https://reviews.llvm.org/D34082#942420, @anemet wrote: > @modocache, @davide, are you guys sure this feature is working? The test > does not actually check whether hotness is included in the remarks and when I > run it manually they are missing. In

[PATCH] D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array

davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Driver/ToolChains/Gnu.cpp:2467 const Generic_GCC::GCCVersion = GCCInstallation.getVersion(); bool UseInitArrayDefault =

[PATCH] D33852: Enable __declspec(selectany) on linux

davide added inline comments. Comment at: include/clang/Basic/Attr.td:2421 -def SelectAny : InheritableAttr, TargetSpecificAttr { +def SelectAny : InheritableAttr, TargetSpecificAttr { let Spellings = [Declspec<"selectany">, GCC<"selectany">]; rnk wrote: >

[PATCH] D33852: Enable __declspec(selectany) on linux

davide added inline comments. Comment at: include/clang/Basic/Attr.td:2421 -def SelectAny : InheritableAttr, TargetSpecificAttr { +def SelectAny : InheritableAttr, TargetSpecificAttr { let Spellings = [Declspec<"selectany">, GCC<"selectany">]; Prazek wrote:

[PATCH] D33852: Enable __declspec(selectany) on linux

davide added a comment. In https://reviews.llvm.org/D33852#772230, @martell wrote: > @Prazek it was not disabled by mistake. > This was a MS extension and it appeared as though the test case should have > been for windows. > I updated it and made the extension available to windows only.

[PATCH] D33852: Enable __declspec(selectany) on linux

davide added a comment. If you take my example, and you pass `-target x86_64-pc-win32-macho`: On clang-3.8, `TinkyWinky` is lowered to a GV with `weak_odr` linkage: $ clang++ 1.cpp -o - -emit-llvm -S -fms-extensions -target x86_64-pc-win32-macho ; ModuleID = '1.cpp' target datalayout =

[PATCH] D33852: Enable __declspec(selectany) on linux

davide added a comment. I think lowering `__declspec(selectany)` to a `comdat` for GVs on ELF platform is actually reasonable. I don't know what happens on Mach-O (as far as I can tell they don't have a real notion of COMDAT, they use coalesced symbols, but I'm not an expert of the format so

[PATCH] D33692: [ThinLTO] Wire up ThinLTO and new PM

davide added a comment. LGTM once the other patch lands. https://reviews.llvm.org/D33692 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31508: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends

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

[PATCH] D31508: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends

davide added a comment. clang -cc1 crashes if an invalid reloc model is passed (via `-mreloc-model=`), see https://bugs.llvm.org/show_bug.cgi?id=32490 I'm not sure if the bug was already there or this refactoring exposed it. Either way, I'm going to submit a patch to get this fixed. Ideally, we

[PATCH] D28799: [llvm-objdump tests] Copy the inputs of tests closer to tests.

davide added a comment. In https://reviews.llvm.org/D28799#648062, @ioeric wrote: > @davide I think this change makes sense. I'll accept this to unbreak our > internal build. Let us know if you have any concern. Yes, makes sense. Thanks. Repository: rL LLVM