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

[Davide Italiano via Phabricator via cfe-commits]

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 of the optimizer wouldn't be exposed as 
frontend flags.

2. I really don't think metadata is the correct way of conveying this 
information to the optimizer. Among all the other problems, metadata can be 
stripped willy-nilly. Maybe a function attribute will work better?

Some alternatives which I don't like, but I can't currently propose anything 
better are:

1. Having a separate optimization pipeline that you use in these cases
2. Having an instrumentation pass that annotates your CFG to prevent passes 
from destroying coverage signal. That said, I'm not really familiar to comment 
on whether this is feasible or not.

Please note that completely disabling SimplifyCFG will result in non-trivial 
performance hits (at least, in my experience), so you might want to evaluate 
this carefully.
@chandlerc might have thoughts on how to address this.


https://reviews.llvm.org/D43423



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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] D33852: Enable __declspec(selectany) on linux

[Davide Italiano via Phabricator via cfe-commits]

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 you might want to ask somebody who's familiar with it). I think Reid 
should sign-off this change anyway.


https://reviews.llvm.org/D33852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-pc-windows-macho"
  
  @TinkyWinky = weak_odr global i32 0, align 4
  
  !llvm.module.flags = !{!0}
  !llvm.ident = !{!1}
  
  !0 = !{i32 1, !"PIC Level", i32 2}
  !1 = !{!"clang version 3.8.0 (tags/RELEASE_380/final)"}

On clang-5.0 (trunk, release+assert), the compiler just crashes :(

  $ ../clang++ 1.cpp -o - -emit-llvm -S -fms-extensions -target 
x86_64-pc-win32-macho
  clang-5.0: ../tools/clang/lib/CodeGen/CodeGenModule.cpp:479: void 
clang::CodeGen::CodeGenModule::Release(): Assertion `(LangOpts.ShortWChar || 
llvm::TargetLibraryInfoImpl::getTarget
  WCharSize(Target.getTriple()) == Target.getWCharWidth() / 8) && "LLVM wchar_t 
size out of sync"' failed.
  #0 0x0206f67a llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x206f67a)
  #1 0x0206d46e llvm::sys::RunSignalHandlers() 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x206d46e)
  #2 0x0206d5bc SignalHandler(int) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x206d5bc)
  #3 0x7f72dc473c30 __restore_rt (/lib64/libpthread.so.0+0x10c30)
  #4 0x7f72dafdf765 __GI_raise (/lib64/libc.so.6+0x34765)
  #5 0x7f72dafe136a __GI_abort (/lib64/libc.so.6+0x3636a)
  #6 0x7f72dafd7f97 __assert_fail_base (/lib64/libc.so.6+0x2cf97)
  #7 0x7f72dafd8042 (/lib64/libc.so.6+0x2d042)
  #8 0x022cab88 clang::CodeGen::CodeGenModule::Release() 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x22cab88)
  #9 0x028fbc77 (anonymous 
namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x28fbc77)
  #10 0x028fa965 
clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x28fa965)
  #11 0x02ccdf38 clang::ParseAST(clang::Sema&, bool, bool) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x2ccdf38)
  #12 0x028f9bbc clang::CodeGenAction::ExecuteAction() 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x28f9bbc)
  #13 0x025bae0e clang::FrontendAction::Execute() 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x25bae0e)
  #14 0x0258c746 
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x258c746)
  #15 0x0264ac7b 
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0x264ac7b)
  #16 0x00aff078 cc1_main(llvm::ArrayRef, char const*, 
void*) (/home/davide/work/llvm/build-release/bin/clang-5.0+0xaff078)
  #17 0x00a92775 main 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0xa92775)
  #18 0x7f72dafcb731 __libc_start_main (/lib64/libc.so.6+0x20731)
  #19 0x00afb7e9 _start 
(/home/davide/work/llvm/build-release/bin/clang-5.0+0xafb7e9)
  Stack dump:
  0.  Program arguments: /home/davide/work/llvm/build-release/bin/clang-5.0 
-cc1 -triple x86_64-pc-windows-macho -emit-llvm -disable-free -main-file-name 
1.cpp -mrelocation-model
  pic -pic-level 2 -mthread-model posix -mdisable-fp-elim -fmath-errno 
-masm-verbose -mconstructor-aliases -target-cpu x86-64 -dwarf-column-info 
-debugger-tuning=gdb -coverage-notes-f
  ile /home/davide/work/llvm/build-release/bin/decl/-.gcno -resource-dir 
/home/davide/work/llvm/build-release/lib/clang/5.0.0 -internal-isystem 
/home/davide/work/llvm/build-release/li
  b/clang/5.0.0/include -fdeprecated-macro -fdebug-compilation-dir 
/home/davide/work/llvm/build-release/bin/decl -ferror-limit 19 -fmessage-length 
181 -fms-extensions -fms-compatibili
  ty -fms-compatibility-version=18 -std=c++11 -fno-threadsafe-statics 
-fdelayed-template-parsing -fobjc-runtime=gcc -fcxx-exceptions -fexceptions 
-fdiagnostics-show-option -fcolor-dia
  gnostics -o - -x c++ 1.cpp
  1.   parser at end of file
  2.  Per-file LLVM IR generation
  clang-5.0: error: unable to execute command: Aborted (core dumped)
  clang-5.0: error: clang frontend command failed due to signal (use -v to see 
invocation)
  clang version 5.0.0 (trunk 304592) (llvm/trunk 304594)
  Target: x86_64-pc-windows-macho


https://reviews.llvm.org/D33852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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. (MSVC, 
> Cygwin, Mingw etc)
>  It didn't expect anyone would want a MS extension for a linux target.


This assumption is not quite right. Also GCC, when targeting Linux, provides a 
`-fms-extensions` flag.
There's a bunch of software out there that unfortunately has to target ELF and 
build with `-fms-extensions` on, e.g. PS4.


https://reviews.llvm.org/D33852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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:
> majnemer wrote:
> > selectany should work on targets other than "x86", "x86_64", "arm", 
> > "thumb", etc. I think it is only necessary to require that it be a COFF or 
> > ELF target.
> Should we allow other OSes than Win32 and Linux?
I guess everything ELF should be allowed.


https://reviews.llvm.org/D33852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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:
> Prazek wrote:
> > rnk wrote:
> > > davide wrote:
> > > > Prazek wrote:
> > > > > majnemer wrote:
> > > > > > selectany should work on targets other than "x86", "x86_64", "arm", 
> > > > > > "thumb", etc. I think it is only necessary to require that it be a 
> > > > > > COFF or ELF target.
> > > > > Should we allow other OSes than Win32 and Linux?
> > > > I guess everything ELF should be allowed.
> > > Why not use weak_odr / linkonce_odr on MachO? Microsoft builds Office for 
> > > Mac and I suspect they use `__declspec(selectany)`.
> > I think this is what would happen right now. The question is - should we 
> > warn about using declspec on macho? Beause not using comdat looks like "not 
> > supporting" it, but I am not sure about it.
> I'm pretty sure weak_odr / linkonce_odr with ld64 on macho are the same as 
> having a comdat. LLVM didn't always have comdats, but it's supported inline 
> functions for a very long time. We should support selectany there.
I agree with @rnk here.


https://reviews.llvm.org/D33852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 Italiano via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 in 
case you need any help reproducing the issue.
cc: @shafik


Repository:
  rC Clang

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 the conversation started with this message: 
> https://lists.llvm.org/pipermail/cfe-dev/2018-August/058842.html
>  I think that the solution to that problem would be exactly this patch, i.e. 
> to put the methods in the proper order during import.
>
> Are the failing lldb tests related to ObjC code?


Some of them are, see e.g. 
http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/11972/console
@shafik is the person who's driving better lldb C++ support on our side, so you 
might consider chatting with him about this.
I just got the bots in a greener state again.


Repository:
  rC Clang

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 not able to reproduce the problem on Linux. 
> Could you please provide a link to a buildbot failure or some environment 
> description so I can reproduce the issue or, at least, take a look?


http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/11972/console
It's unfortunate because this doesn't show a backtrace. I'm afraid at this 
point the easiest way to reproduce this is on MacOS directly.


Repository:
  rC Clang

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Davide Italiano via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 landing patches to ASTImporter without 
proper reviews and testing as I am of this very instance. 
I really think you should consider asking reviews to somebody who's familiar 
with clang and test lldb as it's the main client of this functionality we have 
in tree.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53818/new/

https://reviews.llvm.org/D53818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 this is not the first patch this has come up 
> > on. I'm worried about the broader trend of landing patches to ASTImporter 
> > without proper reviews and testing as I am of this very instance. I really 
> > think you should consider asking reviews to somebody who's familiar with 
> > clang and test lldb as it's the main client of this functionality we have 
> > in tree.
>
> @davide I am terribly sorry about this issue. I just applied this patch on my 
> Linux box, and it did not failed those tests which failed on the green lab 
> buildbots. Could you please refer to the Linux buildbots which failed? Seems 
> like there is a significant difference between the testsuites on Linux and 
> MacOS. Our primary target is Linux and we do run the LLDB tests before we 
> accept any commit into our fork and only then we continue with upstreaming to 
> Phabricator.


I have no Linux bots access, this fails on macos (looks on 
http://lab.llvm.org:8080/green/). Although now it's too late because the logs 
are gone.

> Our platform in our CI is Linux only, but we are in the middle of getting a 
> hosted MacOS to support our CI. I do hope that in the near future there will 
> be no such issues. In the meantime, is there a way for us to execute a green 
> lab MacOS build bot with a specific patch before a commit? If that is not 
> possible, then we can just promise that we will monitor your buildbots and we 
> will do the revert.

I'm not aware of a way of pre-commit testing this hosted on llvm.org. I 
generally run on my machine and check the bots output.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53818/new/

https://reviews.llvm.org/D53818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 for XCOFF");
+  return false;

hubert.reinterpretcast wrote:
> JDevlieghere wrote:
> > jasonliu wrote:
> > > JDevlieghere wrote:
> > > > jasonliu wrote:
> > > > > apaprocki wrote:
> > > > > > No need to be `sorry:` :) This should probably just say `error: 
> > > > > > XCOFF is unimplemented` to be more direct in case anything is 
> > > > > > expecting "error:" in the output.
> > > > > Sure. Will address in next revision.
> > > > Just bundle this with the WASM case, the error message is correct for 
> > > > both.
> > > I think they are different. 
> > > The error message for WASM seems to suggest that it will never ever get 
> > > supported on WASM. 
> > > But it is not the case for XCOFF, we want to indicate that it is not 
> > > implemented yet.  
> > I don't think the error message suggests that at all, and it's definitely 
> > not true. At this point neither XCOFF nor WASM is supported, and that's 
> > exactly what the log message says.
> > 
> I agree that the error message for WASM does not indicate that the lack of 
> support is inherent or intended to be permanent; however, it is not 
> indicative either of an intent to implement the support. I am not sure what 
> the intent is for WASM, but I do know that the intent for XCOFF is to 
> eventually implement the support. I do not see how using an ambiguous message 
> in this commit (when we know what the intent is) is superior to the 
> alternative of having an unambiguous message.
I think we should keep this consistent with the other target so my vote is for 
grouping XCOFF with WASM. After all, if it's going to be implemented soon, the 
message will go away :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58930/new/

https://reviews.llvm.org/D58930



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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] D65116: [Driver] Set the default win32-macho debug format to DWARF

[Davide Italiano via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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, this is failing building LibCxx testcases with a linker 
error:

  Build Command Output:
  Undefined symbols for architecture x86_64:
"std::__1::__vector_base_common::__vector_base_common()", referenced 
from:
std::__1::__vector_base 
>::__vector_base() in main.o
std::__1::__vector_base, std::__1::allocator >, 
std::__1::allocator, 
std::__1::allocator > > >::__vector_base() in main.o
  ld: symbol(s) not found for architecture x86_64
  clang-7: error: linker command failed with exit code 1 (use -v to see 
invocation)
  make: *** [a.out] Error 1


Repository:
  rCXX libc++

https://reviews.llvm.org/D48892



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 https://reviews.llvm.org/D40678, I am 
> filtering out remarks with no hotness when any threshold is set all the 
> remarks are filtered out in this new test.
>
> So either the test is incorrect or somehow with sample-based profiling we 
> don't get hotness info.
>
> Any ideas?  I am inclined to just remove this test for now and file a bug to 
> fix this in order to unblock https://reviews.llvm.org/D40678.


I don't know, I haven't reviewed this feature, but I can take a look later 
today.


https://reviews.llvm.org/D34082



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 issue is non-existent on arm64, but it could be still a 
thing on x86_64. 
If a function cannot be encoded in CU, we need the compiler to emit both (where 
the CU is a magic value that points to the dwarf unwind info for that function).

tl;dr: this is "a nice cleanup", and potentially the bincompat issue might be 
cone, but the cost of breaking programs in the wild is IMHO too high.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122258/new/

https://reviews.llvm.org/D122258

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 arm64 then? And 
> have it be opt-in just for x86_64?

I think it's fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122258/new/

https://reviews.llvm.org/D122258

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
  
  @implementation Bar
  @end
  
  id bar()
  {
  return [Bar class];
  }

and

  #import 
  
  @interface Foo : NSObject
  @end
  
  @implementation Foo
  @end
  
  id foo()
  {
  return [Foo class];
  }

built with:

`xcrun --sdk macosx.internal clang++ foo.m bar.m -o lib.dylib -lobjc 
-dynamiclib -O2 -Wl,-dead_strip`

if you run nm on it, you'll see 2 copies of all the protocol metadata:

  $ nm -j lib.dylib | uniq -c | grep 2
 2 __OBJC_$_PROP_LIST_NSObject
 2 __OBJC_$_PROTOCOL_INSTANCE_METHODS_NSObject
 2 __OBJC_$_PROTOCOL_INSTANCE_METHODS_OPT_NSObject
 2 __OBJC_$_PROTOCOL_METHOD_TYPES_NSObject

This is because some metadata is emitted with `internal` linkage and `default` 
visibility, e.g.:

  @"_OBJC_$_PROP_LIST_NSObject" = internal global { i32, i32, [4 x 
%struct._prop_t] } 

This patch changes `clang` to emit protocol metadata as `weak hidden`, so that 
`ld64` can deduplicate them.

Fixes rdar://problem/85042564


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113456

Files:
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/test/CodeGenObjC/direct-properties.m
  clang/test/CodeGenObjC/encode-test-2.m
  clang/test/CodeGenObjC/metadata-class-properties.m
  clang/test/CodeGenObjC/metadata-symbols-32.m
  clang/test/CodeGenObjC/metadata-symbols-64.m
  clang/test/CodeGenObjC/property-category-impl.m
  clang/test/CodeGenObjC/property-list-in-class.m
  clang/test/CodeGenObjC/property-list-in-extension.m
  clang/test/CodeGenObjC/protocols.m
  clang/test/CodeGenObjC/sections.m

Index: clang/test/CodeGenObjC/sections.m
===
--- clang/test/CodeGenObjC/sections.m
+++ clang/test/CodeGenObjC/sections.m
@@ -34,7 +34,7 @@
   return [I class] == @protocol(P);
 }
 
-// CHECK-COFF: @"_OBJC_$_CLASS_METHODS_I" = private
+// CHECK-COFF: @"_OBJC_$_CLASS_METHODS_I" = weak hidden
 // CHECK-COFF: @"OBJC_CLASSLIST_SUP_REFS_$_" = private {{.*}}, section ".objc_superrefs$B"
 // CHECK-COFF: @OBJC_SELECTOR_REFERENCES_ = private {{.*}}, section ".objc_selrefs$B"
 // CHECK-COFF: @"OBJC_CLASSLIST_REFERENCES_$_" = private {{.*}}, section ".objc_classrefs$B"
@@ -47,7 +47,7 @@
 // CHECK-COFF: @"OBJC_LABEL_NONLAZY_CATEGORY_$" = private {{.*}}, section ".objc_nlcatlist$B"
 // CHECK-COFF: !{{[0-9]+}} = !{i32 1, !"Objective-C Image Info Section", !".objc_imageinfo$B"}
 
-// CHECK-ELF: @"_OBJC_$_CLASS_METHODS_I" = private
+// CHECK-ELF: @"_OBJC_$_CLASS_METHODS_I" = weak hidden
 // CHECK-ELF: @"OBJC_CLASSLIST_SUP_REFS_$_" = private {{.*}}, section "objc_superrefs"
 // CHECK-ELF: @OBJC_SELECTOR_REFERENCES_ = private {{.*}}, section "objc_selrefs"
 // CHECK-ELF: @"OBJC_CLASSLIST_REFERENCES_$_" = private {{.*}}, section "objc_classrefs"
@@ -60,7 +60,7 @@
 // CHECK-ELF: @"OBJC_LABEL_NONLAZY_CATEGORY_$" = private {{.*}}, section "objc_nlcatlist"
 // CHECK-ELF: !{{[0-9]+}} = !{i32 1, !"Objective-C Image Info Section", !"objc_imageinfo"}
 
-// CHECK-MACHO: @"_OBJC_$_CLASS_METHODS_I" = internal {{.*}}, section "__DATA, __objc_const"
+// CHECK-MACHO: @"_OBJC_$_CLASS_METHODS_I" = weak hidden {{.*}}, section "__DATA, __objc_const"
 // CHECK-MACHO: @"OBJC_CLASSLIST_SUP_REFS_$_" = private {{.*}}, section "__DATA,__objc_superrefs,regular,no_dead_strip"
 // CHECK-MACHO: @OBJC_SELECTOR_REFERENCES_ = internal {{.*}}, section "__DATA,__objc_selrefs,literal_pointers,no_dead_strip"
 // CHECK-MACHO: @"OBJC_CLASSLIST_REFERENCES_$_" = internal {{.*}}, section "__DATA,__objc_classrefs,regular,no_dead_strip"
Index: clang/test/CodeGenObjC/protocols.m
===
--- clang/test/CodeGenObjC/protocols.m
+++ clang/test/CodeGenObjC/protocols.m
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s | FileCheck %s
 
-// CHECK: @"_OBJC_$_PROTOCOL_METHOD_TYPES_P1" = internal global
+// CHECK: @"_OBJC_$_PROTOCOL_METHOD_TYPES_P1" = weak hidden global
 // CHECK: @[[PROTO_P1:"_OBJC_PROTOCOL_\$_P1"]] = weak hidden
 // CHECK: @[[LABEL_PROTO_P1:"_OBJC_LABEL_PROTOCOL_\$_P1"]] = weak hidden global %{{.*}}* @[[PROTO_P1]]
 // CHECK: @[[PROTO_P2:"_OBJC_PROTOCOL_\$_P2"]] = weak hidden
Index: clang/test/CodeGenObjC/property-list-in-extension.m
===
--- clang/test/CodeGenObjC/property-list-in-extension.m
+++ clang/test/CodeGenObjC/property-list-in-extension.m
@@ -18,7 +18,7 @@
 // Metadata for _myprop should be present, and PROP_LIST for Foo should have
 // only one entry.
 // CHECK: = private unnamed_addr constant [12 x i8] c"Ti,V_myprop\00",
-// CHECK: @"_OBJC_$_PROP_LIST_Foo" = internal global { i32, i32, [1 x %struct._prop_t] }
+// CHECK: @"_OBJC_$_PROP_LIST_Foo" = wea

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

[Davide Italiano via Phabricator via cfe-commits]

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 it does not want to support it.
>
> The main purpose is to reduce target differences: currently both COFF and ELF 
> can use dso_local. If Mach-O doesn't like dso_local, there is an additional 
> difference.

The model for `dso_local` is different on Mach-O. While it's a laudable goal to 
unify the three formats, we might want to consider to not push these changes 
willy-nilly.
We should consider investigating what's broken but the reality is that this 
breaks xnu so we need to revert this for the time being.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98458/new/

https://reviews.llvm.org/D98458

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D98458

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 SINCE LAST ACTION
  https://reviews.llvm.org/D140224/new/

https://reviews.llvm.org/D140224

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 this patch until we find an agreement.
>
> D139717  (this patch reverts) was pushed 
> when I made valid comments which were ignored. I did not complain for that.
>
> I don't mind if you work around `-Xcctests` in a similar way.

Working around 3 cases creates more complexity than it fixes.
We're also not providing a deprecation path for users. This needs to be 
discussed more thoroughly. I'll go ahead and revert to the previous status.

Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140224/new/

https://reviews.llvm.org/D140224

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

davide added a comment.

In D140224#4014245 , @MaskRay wrote:

> In D140224#4014234 , @davide wrote:
>
>> 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 this patch until we find an agreement.
>>>
>>> D139717  (this patch reverts) was pushed 
>>> when I made valid comments which were ignored. I did not complain for that.
>>>
>>> I don't mind if you work around `-Xcctests` in a similar way.
>>
>> Working around 3 cases creates more complexity than it fixes.
>> We're also not providing a deprecation path for users. This needs to be 
>> discussed more thoroughly. I'll go ahead and revert to the previous status.
>>
>> Thanks.
>
> Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that 
> such `-X*` has `-Wunused-command-line-argument` warning for many many years.
> I'm not sure how is considered insufficient.
>
> "Working around 3 cases creates more complexity than it fixes." the number 
> isn't that high. By enumerating the misuse, we have a valid path to remove 
> all workarounds as misuses are fixed.
> This made some forward progress.

You can't just remove options willy-nilly. This is not how drivers work. The 
warning says "unused", it doesn't say "it goes away".
If we want to provide a path forward, we first need to reinstate this, then 
change the warning, then remove (in 1 year or something).
That's how transitions work.

HTH.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140224/new/

https://reviews.llvm.org/D140224

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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  , so I don't think it needs more 
>>> approval.
>>
>> We have projects that are failing because of -Xlinker and I'm not too 
>> excited about walking through every project we have to find more examples. 
>> Can we please have a reprieve on deprecating this until every project in the 
>> world that uses these flags that have existed for a very long time can have 
>> a chance to at least get timely notification? Why is this any different than 
>> anything else that gets deprecated?
>
> Thank you for your reply! If there are more issues 
> -Xlinker/-Xparser/-Xcctests, ok, I think I'm fine with a Joined -X, but 
> ideally we can figure out a way to apply that only to Apple platforms.
>
> (FWIW I feel sad when I made valid objection to D139717 
> , but that patch landed very soon after 
> your colleague approved it, leaving me no time to object.
> My other objection included the insufficient commit message `-Xfoo` instead 
> of what's actually used, `internal builds` which seems to hint an internal 
> workaround, where the tests are located)

It's not an internal workaround.

In D140224#4014291 , @MaskRay wrote:

> In D140224#4014246 , @davide wrote:
>
>> In D140224#4014245 , @MaskRay 
>> wrote:
>>
>>> In D140224#4014234 , @davide 
>>> wrote:
>>>
 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 this patch until we find an agreement.
>
> D139717  (this patch reverts) was 
> pushed when I made valid comments which were ignored. I did not complain 
> for that.
>
> I don't mind if you work around `-Xcctests` in a similar way.

 Working around 3 cases creates more complexity than it fixes.
 We're also not providing a deprecation path for users. This needs to be 
 discussed more thoroughly. I'll go ahead and revert to the previous status.

 Thanks.
>>>
>>> Have you seen https://reviews.llvm.org/D139717#4001712 I have analyzed that 
>>> such `-X*` has `-Wunused-command-line-argument` warning for many many years.
>>> I'm not sure how is considered insufficient.
>>>
>>> "Working around 3 cases creates more complexity than it fixes." the number 
>>> isn't that high. By enumerating the misuse, we have a valid path to remove 
>>> all workarounds as misuses are fixed.
>>> This made some forward progress.
>>
>> You can't just remove options willy-nilly. This is not how drivers work. The 
>> warning says "unused", it doesn't say "it goes away".
>> If we want to provide a path forward, we first need to reinstate this, then 
>> change the warning, then remove (in 1 year or something).
>> That's how transitions work.
>>
>> HTH.
>
> I almost agree with you, but only for actually-used options which do real 
> work instead of ignored options.
> If it is pure compatibility option, I wish that there is public references 
> instead of pure internal build uses.
> For Joined `-X` I am unsure I want to take your opinion.
>
> Neither sourcegraph nor search/Debian Code Search shows anything about 
> `-Xcctests`.

Debian code search and source graphs aren't the universe. There's a lot of code 
in the world that doesn't get published for a variety of reasons.
The gain of this change is very little compared to the pain introduced. Let's 
give people a window where they can transition, then screw them over. not the 
other way around.

Best,

-

D

> Separate `-Xlinker ` still exists: not removed by this patch. `-Xlinker=` is 
> invalid. Note that many references on a code search website reveal libtool 
> uses instead of compiler driver uses.
> If your internal build uses many `-X*` forms, I am fine with ignoring them. 
> If there are so many that you want to ignore `-X*`, I think I am fine as 
> well, but ideally restricted to Apple platforms (ideally show some external 
> usage examples).




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140224/new/

https://reviews.llvm.org/D140224

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 expressions for list initialization.");
   SourceRange FullRange = SourceRange(TyBeginLoc, RParenOrBraceLoc);

I'd rephrase this as "List initialization must have exactly one expression"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138947/new/

https://reviews.llvm.org/D138947

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 complexity to keep this 
around. Removing is more annoying than anything else. 
I agree on the test, that has to be added.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139717/new/

https://reviews.llvm.org/D139717

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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.
>
> Which `-Xfoo` is used? Do your internal builds reserve `-Xfoo` as a 
> placeholder to do other non-upstream work? Then you can consider `-Gfoo` if 
> your builds don't need the mips semantics.
> Is it possible to clean up the previously-ignored-with-a-warning `-Xfoo`?

@MaskRay -- we can discuss options but we can't just "kill" options willy-nilly 
just because you don't use them.
A better way of doing this would be marking them as deprecated, wait one 
release and then remove. Going cold turkey breaks clients you never heard of, 
and you didn't even warn.

Let's add a test and revert this patch, then we can continue the discussion on 
how to phase this out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139717/new/

https://reviews.llvm.org/D139717

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

davide added a comment.

In D139717#3997193 , @MaskRay wrote:

> In D139717#3991765 , @rsundahl 
> wrote:
>
>> 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.
>>>
>>> Which `-Xfoo` is used? Do your internal builds reserve `-Xfoo` as a 
>>> placeholder to do other non-upstream work? Then you can consider `-Gfoo` if 
>>> your builds don't need the mips semantics.
>>
>> We use -Xparser
>
> There are two possibilities.
>
> First, your downstream projects incorrectly pass a previously-ignored 
> `-Xparser`. Then it seems the right call is to remove it.

It is not. You first deprecate and then you remove. This commit needs to be 
reverted.

HTH,

-

D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139717/new/

https://reviews.llvm.org/D139717

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 wide-spread option used by 
>> many packages. Whether or not GCC supports it is not relevant. Please do not 
>> remove options just because you do not use them.
>
> Can you give an example how they use `-Xcompiler`?
>
>   % gcc -Xcompiler,-fpic -c a.c
>   gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
>   % gcc -Xcompiler -fpic -c a.c
>   gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean 
> ‘--compile’?
>
> My commit message clearly says why the joined form is awkward and should be 
> removed.
> It seems that the many occurrences you found are likely for GNU libtool 
> (`-Xcompiler foo` `-Wc,foo`), not for Clang Driver.

This is not about the philosophical correctness of the patch, it's about the 
transition period and allowing consumers to migrate.
If you want remove options, provide a deprecation window, and then remove. 
Noone is objecting about that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139717/new/

https://reviews.llvm.org/D139717

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

davide added a comment.

In D139717#4001702 , @MaskRay wrote:

> In D139717#4001688 , @davide wrote:
>
>> 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 wide-spread option used by 
 many packages. Whether or not GCC supports it is not relevant. Please do 
 not remove options just because you do not use them.
>>>
>>> Can you give an example how they use `-Xcompiler`?
>>>
>>>   % gcc -Xcompiler,-fpic -c a.c
>>>   gcc: error: unrecognized command-line option ‘-Xcompiler,-fpic’
>>>   % gcc -Xcompiler -fpic -c a.c
>>>   gcc: error: unrecognized command-line option ‘-Xcompiler’; did you mean 
>>> ‘--compile’?
>>>
>>> My commit message clearly says why the joined form is awkward and should be 
>>> removed.
>>> It seems that the many occurrences you found are likely for GNU libtool 
>>> (`-Xcompiler foo` `-Wc,foo`), not for Clang Driver.
>>
>> This is not about the philosophical correctness of the patch, it's about the 
>> transition period and allowing consumers to migrate.
>> If you want remove options, provide a deprecation window, and then remove. 
>> Noone is objecting about that.
>
> `-Xparser` has always been leading to such a warning: `warning: argument 
> unused during compilation: '-Xparser' [-Wunused-command-line-argument]`, 
> perhaps since forever when the option was added in the first place?
> The message is different from `... deprecation ...` but isn't it sufficient 
> as well?

Yes. it is. "unused" doesn't mean "will go away".
Sometimes if you pass linker flags to the compiler you get the same "unused" 
warning. Noone expects them to go away.

Hope this helps.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139717/new/

https://reviews.llvm.org/D139717

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 shouldn't keep two codepath in sync (as we do already for 
`getCodeModel()`, but that's the status quo). I may decide to refactor the 
whole handling to avoid this sync issue, but that won't be today.


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 Italiano via Phabricator via cfe-commits]

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] D28799: [llvm-objdump tests] Copy the inputs of tests closer to tests.

[Davide Italiano via Phabricator via cfe-commits]

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

https://reviews.llvm.org/D28799



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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 &V = GCCInstallation.getVersion();
   bool UseInitArrayDefault =
   getTriple().getArch() == llvm::Triple::aarch64 ||

this is really a mess.


https://reviews.llvm.org/D35337



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits