[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D125624#3552770 , @tstellar wrote:

> In D125624#3552463 , @ruiu wrote:
>
>>> OK, as I mentioned. I think we need an attorney to review this change and 
>>> confirm that it actually accomplishes this goal.
>>
>> Can you add an attorney as a reviewer of this change so that we can proceed?
>
> Yes, I've notified @tonic and she will reach out to the attorney.

@tstellar @tonic Did you guys get any response from the attorney?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> OK, as I mentioned. I think we need an attorney to review this change and 
> confirm that it actually accomplishes this goal.

Can you add an attorney as a reviewer of this change so that we can proceed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D125624#3552284 , @khchen wrote:

> IMO, maybe we could keep the DLLVM_BINUTILS_INCDIR option support but default 
> is using the Plugin.h?

We can, but I wonder what is the motivation of doing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> So what happens if LLVMgold.so  uses one of the new structs or constants and 
> then is built and run on system where binutils is old enough to not have 
> these new structs  and constants?

The same thing can happen with GCC and binutils. Imagine that you install a 
newer version of GCC binary to your system and use it with an older version of 
binutils ld which doesn't know anything about new structs and constants. The 
plugin should still work, as changes to this file must be made in such a way 
that that don't break ABI. The whole point of this interface is decoupling 
compiler versions from linker versions. So it's backward compatible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

The motivation of doing this is to be able to build LLVMgold.so without 
binutils' source files and make it clear that LLVMgold.so does not include any 
GPL code.

The header file defines the public interface between linker plugins and 
compilers (and other tools such as `ar` which has to read symbol table of LTO 
object files). New structs or constants may be added to this header, but the 
existing ones will never be deleted or altered in such a way that that breaks 
compatibility. So, the declarations in this file aren't different from other 
structs and types that are defined the same as they are in GNU systems. We 
already have lots of such structs and types in llvm/include, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I'm not sure if my credential is still valid. Do you mind if I ask you to 
submit this for me?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Does it? What is your test command?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D125624: [gold] Remove an external dependency to GNU binutils' header file

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.
Herald added a subscriber: Enna1.



Comment at: llvm/tools/gold/gold-plugin.cpp:44-52
 // FIXME: remove this declaration when we stop maintaining Ubuntu Quantal and
 // Precise and Debian Wheezy (binutils 2.23 is required)
 #define LDPO_PIE 3
 
 #define LDPT_GET_SYMBOLS_V3 28
 
 // FIXME: Remove when binutils 2.31 (containing gold 1.16) is the minimum

pcc wrote:
> Can we remove this stuff now?
I believe so, but I'll do that in another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125624

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

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: lld/ELF/Config.h:132
   callGraphProfile;
+  llvm::TargetLibraryInfoImpl::VectorLibrary VectLib;
   bool allowMultipleDefinition;

We name variables after their corresponding command line flags, so this should 
be `ltoVectorLibrary`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77484



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

[PATCH] D70048: [LLD] Add NetBSD support as a new flavor of LLD (nb.lld)

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

lld is not that Linux-centric as you implied in your comment. We have code for 
FreeBSD, OpenBSD, AMDGPU and Fuchsia (which isn't even a POSIX system), and 
adding code for NetBSD is OK and is appreciated. However, the thing we've been 
trying to avoid is to hard-code a platform-specific knowledge such as library 
paths to the linker. Combined with the fact that lld doesn't provide a way to 
enable/disable targets at build-time, all ld.lld executables (sometimes 
installed as /usr/bin/ld) work the same way no matter where they are run on. 
That helps those who do cross compilation a lot. That is a very appreciated 
property of our linker, and I don't think I want make an exception for NetBSD. 
Even on NetBSD, lld should behave the same as running on other kind of hosts.

Also, frankly speaking, having a driver only for NetBSD seems very odd to me. 
We would have 5 drivers for MSVC link.exe, MinGW, ELF-based systems, macOS, and 
NetBSD? That doesn't feel right. If you strongly feel that the linker should 
know the library paths and the like, it shouldn't be too hard to write a shell 
script wrapper to wrap lld.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70048



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

[PATCH] D70919: [Hexagon] Avoid passing unsupported options to lld when -fuse-ld=lld is used

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D70919#1771198 , @sidneym wrote:

> Remove quotes around check-not.
>
> -fuse-ld=lld is the correct usage.  -fuse-ld=ld.lld results in an error 
> message:
>  error: invalid linker name in argument '-fuse-ld=ld.lld'


`-fuse-ld` accepts an absolute path to a linker, so you can pass for example 
`-fuse-ld=/absolute/path/to/ld.lld`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70919



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

[PATCH] D70694: Use InitLLVM in clang-tidy

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7acba29c19a: Use InitLLVM in clang-tidy (authored by ruiu).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70694

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidyForceLinker.h"
 #include "../GlobList.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
@@ -327,7 +328,7 @@
 }
 
 static int clangTidyMain(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::InitLLVM X(argc, argv);
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
   llvm::IntrusiveRefCntPtr BaseFS(


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidyForceLinker.h"
 #include "../GlobList.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
@@ -327,7 +328,7 @@
 }
 
 static int clangTidyMain(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::InitLLVM X(argc, argv);
   CommonOptionsParser OptionsParser(argc, argv, ClangTidyCategory,
 cl::ZeroOrMore);
   llvm::IntrusiveRefCntPtr BaseFS(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70702: Use InitLLVM to setup a pretty stack printer

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f76260dc067: Use InitLLVM to setup a pretty stack printer 
(authored by ruiu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70702

Files:
  clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/clang-query/tool/ClangQuery.cpp
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/tool-template/ToolTemplate.cpp
  clang/tools/arcmt-test/arcmt-test.cpp
  clang/tools/c-index-test/core_main.cpp
  clang/tools/clang-check/ClangCheck.cpp
  clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/utils/TableGen/TableGen.cpp
  llvm/utils/KillTheDoctor/KillTheDoctor.cpp
  llvm/utils/TableGen/TableGen.cpp

Index: llvm/utils/TableGen/TableGen.cpp
===
--- llvm/utils/TableGen/TableGen.cpp
+++ llvm/utils/TableGen/TableGen.cpp
@@ -12,6 +12,7 @@
 
 #include "TableGenBackends.h" // Declares all backends.
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
@@ -266,8 +267,7 @@
 }
 
 int main(int argc, char **argv) {
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   cl::ParseCommandLineOptions(argc, argv);
 
   llvm_shutdown_obj Y;
Index: llvm/utils/KillTheDoctor/KillTheDoctor.cpp
===
--- llvm/utils/KillTheDoctor/KillTheDoctor.cpp
+++ llvm/utils/KillTheDoctor/KillTheDoctor.cpp
@@ -36,6 +36,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -295,8 +296,7 @@
 
 int main(int argc, char **argv) {
   // Print a stack trace if we signal out.
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   llvm_shutdown_obj Y;  // Call llvm_shutdown() on exit.
 
   ToolName = argv[0];
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -10,9 +10,10 @@
 //
 //===--===//
 
-#include "TableGenBackends.h" // Declares all backends.
 #include "ClangASTEmitters.h"
+#include "TableGenBackends.h" // Declares all backends.
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/TableGen/Error.h"
@@ -349,8 +350,7 @@
 }
 
 int main(int argc, char **argv) {
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   cl::ParseCommandLineOptions(argc, argv);
 
   llvm_shutdown_obj Y;
Index: clang/tools/clang-refactor/ClangRefactor.cpp
===
--- clang/tools/clang-refactor/ClangRefactor.cpp
+++ clang/tools/clang-refactor/ClangRefactor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -608,7 +609,7 @@
 } // end anonymous namespace
 
 int main(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::InitLLVM X(argc, argv);
 
   ClangRefactorTool RefactorTool;
 
Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/ToolOutputFile.h"
@@ -303,7 +304,7 @@
 } // anonymous namespace
 
 int main(int argc, const char **argv) {
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
+  

[PATCH] D70702: Use InitLLVM to setup a pretty stack printer

[Rui Ueyama via Phabricator via cfe-commits]

ruiu created this revision.
ruiu added a reviewer: MaskRay.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous.
Herald added projects: clang, LLVM.

InitLLVM does not only save a few lines from main() but also makes the
commands do the right thing for multibyte character pathnames on
Windows (i.e. canonicalize argv's to UTF-8) because of the code we
have in this file:

https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/InitLLVM.cpp#L32

For many LLVM commands, we already have calls of InitLLVM, but there
are still remainings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70702

Files:
  clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/clang-query/tool/ClangQuery.cpp
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/tool-template/ToolTemplate.cpp
  clang/tools/arcmt-test/arcmt-test.cpp
  clang/tools/c-index-test/core_main.cpp
  clang/tools/clang-check/ClangCheck.cpp
  clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
  clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/utils/TableGen/TableGen.cpp
  llvm/utils/KillTheDoctor/KillTheDoctor.cpp
  llvm/utils/TableGen/TableGen.cpp

Index: llvm/utils/TableGen/TableGen.cpp
===
--- llvm/utils/TableGen/TableGen.cpp
+++ llvm/utils/TableGen/TableGen.cpp
@@ -12,6 +12,7 @@
 
 #include "TableGenBackends.h" // Declares all backends.
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
@@ -266,8 +267,7 @@
 }
 
 int main(int argc, char **argv) {
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   cl::ParseCommandLineOptions(argc, argv);
 
   llvm_shutdown_obj Y;
Index: llvm/utils/KillTheDoctor/KillTheDoctor.cpp
===
--- llvm/utils/KillTheDoctor/KillTheDoctor.cpp
+++ llvm/utils/KillTheDoctor/KillTheDoctor.cpp
@@ -36,6 +36,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -295,8 +296,7 @@
 
 int main(int argc, char **argv) {
   // Print a stack trace if we signal out.
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   llvm_shutdown_obj Y;  // Call llvm_shutdown() on exit.
 
   ToolName = argv[0];
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -10,9 +10,10 @@
 //
 //===--===//
 
-#include "TableGenBackends.h" // Declares all backends.
 #include "ClangASTEmitters.h"
+#include "TableGenBackends.h" // Declares all backends.
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/TableGen/Error.h"
@@ -349,8 +350,7 @@
 }
 
 int main(int argc, char **argv) {
-  sys::PrintStackTraceOnErrorSignal(argv[0]);
-  PrettyStackTraceProgram X(argc, argv);
+  llvm::InitLLVM X(argc, argv);
   cl::ParseCommandLineOptions(argc, argv);
 
   llvm_shutdown_obj Y;
Index: clang/tools/clang-refactor/ClangRefactor.cpp
===
--- clang/tools/clang-refactor/ClangRefactor.cpp
+++ clang/tools/clang-refactor/ClangRefactor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -608,7 +609,7 @@
 } // end anonymous namespace
 
 int main(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::InitLLVM X(argc, argv);
 
   ClangRefactorTool RefactorTool;
 
Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Support/Errc.h"
 #include 

[PATCH] D69043: [RFC] Adding time-trace to LLD?

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: lld/ELF/Options.td:361
 
+def time_trace: F<"time-trace">, HelpText<"Record time trace">;
+

aganea wrote:
> Given this option is a candidate for the other LLD drivers, I am wondering if 
> we couldn't have a shared `lld/Common/CommonOpt.td`. @ruiu WDYT?
In many cases command line options are slightly different among drivers. so 
currently we have a completely separated option file for each driver. It looks 
like maintaining each file separately makes things much easier, so I wouldn't 
create a new common file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69043



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

[PATCH] D69043: [RFC] Adding time-trace to LLD?

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I applied this patch and built clang with ThinLTO. Here is the generated file:

$ less bin/clang-10.json 
{"traceEvents":[{"pid":1,"tid":185412,"ph":"X","ts":181,"dur":1441864,"name":"Parse
 input 
files","args":{"detail":""}},{"pid":1,"tid":185412,"ph":"X","ts":12564626,"dur":541606,"name":"OptModule","args":{"detail":"ld-temp.o"}},{"pid":1,"tid":185412,"ph":"X","ts":1454232,"dur":17946238,"name":"LTO","args":{"detail":""}},{"pid":1,"tid":185412,"ph":"X","ts":19461534,"dur":15275,"name":"GC","args":{"detail":""}},{"pid":1,"tid":185412,"ph":"X","ts":19721702,"dur":1241569,"name":"Write
 output 
file","args":{"detail":""}},{"pid":1,"tid":185412,"ph":"X","ts":2,"dur":20963276,"name":"ExecuteLinker","args":{"detail":""}},{"pid":1,"tid":185413,"ph":"X","ts":0,"dur":20963276,"name":"Total
 ExecuteLinker","args":{"count":1,"avg 
ms":20963}},{"pid":1,"tid":185414,"ph":"X","ts":0,"dur":17946237,"name":"Total 
LTO","args":{"count":1,"avg 
ms":17946}},{"pid":1,"tid":185415,"ph":"X","ts":0,"dur":1441864,"name":"Total 
Parse input files","args":{"count":1,"avg 
ms":1441}},{"pid":1,"tid":185416,"ph":"X","ts":0,"dur":1241569,"name":"Total 
Write output file","args":{"count":1,"avg 
ms":1241}},{"pid":1,"tid":185417,"ph":"X","ts":0,"dur":541704,"name":"Total 
OptModule","args":{"count":2,"avg 
ms":270}},{"pid":1,"tid":185418,"ph":"X","ts":0,"dur":15274,"name":"Total 
GC","args":{"count":1,"avg 
ms":15}},{"cat":"","pid":1,"tid":0,"ts":0,"ph":"M","name":"process_name","args":{"name":"ld.lld"}}]}

This file seems much smaller than yours. What am I missing? (I couldn't 
download your file because the server times out perhaps due to the file size.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69043



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

[PATCH] D69043: [RFC] Adding time-trace to LLD?

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

This seems useful. Can I see an example output?

Speaking of the output file, I'd make --time-trace option to take an output 
filename, as an output executable doesn't always have an extension (On Windows 
it's almost always .exe, but on Unix, extension is usually omitted).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69043



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

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368259: [diagtool] Use `operator(Colors)` to print 
out colored output. (authored by ruiu, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65854?vs=213825=214071#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65854

Files:
  cfe/trunk/tools/diagtool/TreeView.cpp


Index: cfe/trunk/tools/diagtool/TreeView.cpp
===
--- cfe/trunk/tools/diagtool/TreeView.cpp
+++ cfe/trunk/tools/diagtool/TreeView.cpp
@@ -20,31 +20,14 @@
 using namespace clang;
 using namespace diagtool;
 
-static bool hasColors(const llvm::raw_ostream ) {
-  if ( != ::errs() &&  != ::outs())
-return false;
-  return llvm::errs().is_displayed() && llvm::outs().is_displayed();
-}
-
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream 
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream ) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +54,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord  : Group.subgroups()) {
@@ -85,12 +67,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord  : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-resetColor();
-out << "\n";
+out << DR.getName() << Colors::RESET << "\n";
   }
 }
   }
@@ -136,13 +116,8 @@
   }
 
   void showKey() {
-if (ShowColors) {
-  out << '\n';
-  setColor(llvm::raw_ostream::GREEN);
-  out << "GREEN";
-  resetColor();
-  out << " = enabled by default\n\n";
-}
+out << '\n' << Colors::GREEN << "GREEN" << Colors::RESET
+<< " = enabled by default\n\n";
   }
 };
 
@@ -182,6 +157,8 @@
 return -1;
   }
 
+  out.enable_colors(out.has_colors());
+
   TreePrinter TP(out);
   TP.Internal = Internal;
   TP.showKey();


Index: cfe/trunk/tools/diagtool/TreeView.cpp
===
--- cfe/trunk/tools/diagtool/TreeView.cpp
+++ cfe/trunk/tools/diagtool/TreeView.cpp
@@ -20,31 +20,14 @@
 using namespace clang;
 using namespace diagtool;
 
-static bool hasColors(const llvm::raw_ostream ) {
-  if ( != ::errs() &&  != ::outs())
-return false;
-  return llvm::errs().is_displayed() && llvm::outs().is_displayed();
-}
-
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream 
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream ) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +54,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord  : Group.subgroups()) {
@@ -85,12 +67,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord  : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu marked an inline comment as done.
ruiu added inline comments.



Comment at: clang/tools/diagtool/TreeView.cpp:167
+  if (!hasColors(out))
+out.enable_colors(false);
+

MaskRay wrote:
> `out.enable_colors(out.has_colors());`
> 
> It looks the function `hasColors` is overengineered. `out` in these files can 
> only be `llvm::outs()`. It doesn't have to check if `llvm::errs()` is 
> connected to a terminal.
Done. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65854



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

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu created this revision.
ruiu added a reviewer: JDevlieghere.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

r368131 introduced this new API to print out messages in colors.
If the colored output is disabled, `operator<<(Colors)` becomes nop.
No functionality change intended.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65854

Files:
  clang/tools/diagtool/TreeView.cpp


Index: clang/tools/diagtool/TreeView.cpp
===
--- clang/tools/diagtool/TreeView.cpp
+++ clang/tools/diagtool/TreeView.cpp
@@ -27,24 +27,13 @@
 }
 
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream 
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream ) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +60,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord  : Group.subgroups()) {
@@ -85,12 +73,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord  : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-resetColor();
-out << "\n";
+out << DR.getName() << Colors::RESET << "\n";
   }
 }
   }
@@ -136,13 +122,8 @@
   }
 
   void showKey() {
-if (ShowColors) {
-  out << '\n';
-  setColor(llvm::raw_ostream::GREEN);
-  out << "GREEN";
-  resetColor();
-  out << " = enabled by default\n\n";
-}
+out << '\n' << Colors::GREEN << "GREEN" << Colors::RESET
+<< " = enabled by default\n\n";
   }
 };
 
@@ -182,6 +163,9 @@
 return -1;
   }
 
+  if (!hasColors(out))
+out.enable_colors(false);
+
   TreePrinter TP(out);
   TP.Internal = Internal;
   TP.showKey();


Index: clang/tools/diagtool/TreeView.cpp
===
--- clang/tools/diagtool/TreeView.cpp
+++ clang/tools/diagtool/TreeView.cpp
@@ -27,24 +27,13 @@
 }
 
 class TreePrinter {
+  using Colors = llvm::raw_ostream::Colors;
+
 public:
   llvm::raw_ostream 
-  const bool ShowColors;
   bool Internal;
 
-  TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(static_cast(Color), false,
- false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
-  }
+  TreePrinter(llvm::raw_ostream ) : out(out), Internal(false) {}
 
   static bool isIgnored(unsigned DiagID) {
 // FIXME: This feels like a hack.
@@ -71,12 +60,11 @@
 out.indent(Indent * 2);
 
 if (enabledByDefault(Group))
-  setColor(llvm::raw_ostream::GREEN);
+  out << Colors::GREEN;
 else
-  setColor(llvm::raw_ostream::YELLOW);
+  out << Colors::YELLOW;
 
-out << "-W" << Group.getName() << "\n";
-resetColor();
+out << "-W" << Group.getName() << "\n" << Colors::RESET;
 
 ++Indent;
 for (const GroupRecord  : Group.subgroups()) {
@@ -85,12 +73,10 @@
 
 if (Internal) {
   for (const DiagnosticRecord  : Group.diagnostics()) {
-if (ShowColors && !isIgnored(DR.DiagID))
-  setColor(llvm::raw_ostream::GREEN);
+if (!isIgnored(DR.DiagID))
+  out << Colors::GREEN;
 out.indent(Indent * 2);
-out << DR.getName();
-resetColor();
-out << "\n";
+out << DR.getName() << Colors::RESET << "\n";
   }
 }
   }
@@ -136,13 +122,8 @@
   }
 
   void showKey() {
-if (ShowColors) {
-  out << '\n';
-  setColor(llvm::raw_ostream::GREEN);
-  out << "GREEN";
-  resetColor();
-  out << " = enabled by default\n\n";
-}
+out << '\n' << Colors::GREEN << "GREEN" << Colors::RESET
+<< " = enabled by default\n\n";
   }
 };
 
@@ -182,6 +163,9 @@
 return -1;
   }
 
+  if (!hasColors(out))
+out.enable_colors(false);
+
   

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367649: Improve raw_ostream so that you can 
write colors using operator (authored by ruiu, committed by 
).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D65564?vs=212959=212968#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65564

Files:
  cfe/trunk/include/clang/AST/ASTDumperUtils.h
  cfe/trunk/lib/Analysis/CFG.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Frontend/TextDiagnostic.cpp
  cfe/trunk/tools/diagtool/TreeView.cpp
  lld/trunk/COFF/Driver.cpp
  lld/trunk/COFF/DriverUtils.cpp
  lld/trunk/Common/ErrorHandler.cpp
  lld/trunk/ELF/Driver.cpp
  lld/trunk/ELF/DriverUtils.cpp
  lld/trunk/include/lld/Common/ErrorHandler.h
  lld/trunk/lib/Driver/DarwinLdDriver.cpp
  lld/trunk/test/COFF/color-diagnostics.test
  lld/trunk/test/ELF/color-diagnostics.test
  lld/trunk/wasm/Driver.cpp
  llvm/trunk/include/llvm/Support/FormattedStream.h
  llvm/trunk/include/llvm/Support/WithColor.h
  llvm/trunk/include/llvm/Support/raw_ostream.h
  llvm/trunk/lib/Support/WithColor.cpp
  llvm/trunk/lib/Support/raw_ostream.cpp
  llvm/trunk/tools/llvm-cov/CoverageReport.cpp
  llvm/trunk/tools/llvm-cov/CoverageViewOptions.h
  llvm/trunk/tools/llvm-cov/RenderingSupport.h
  llvm/trunk/tools/llvm-cov/SourceCoverageViewText.cpp
  llvm/trunk/tools/llvm-cov/llvm-cov.cpp
  llvm/trunk/tools/llvm-mca/Views/TimelineView.cpp
  llvm/trunk/utils/FileCheck/FileCheck.cpp

Index: lld/trunk/include/lld/Common/ErrorHandler.h
===
--- lld/trunk/include/lld/Common/ErrorHandler.h
+++ lld/trunk/include/lld/Common/ErrorHandler.h
@@ -87,7 +87,6 @@
   StringRef errorLimitExceededMsg = "too many errors emitted, stopping now";
   StringRef logName = "lld";
   llvm::raw_ostream *errorOS = ::errs();
-  bool colorDiagnostics = llvm::errs().has_colors();
   bool exitEarly = true;
   bool fatalWarnings = false;
   bool verbose = false;
@@ -102,9 +101,9 @@
   std::unique_ptr outputBuffer;
 
 private:
-  void printHeader(StringRef s, raw_ostream::Colors c, const Twine );
-  void printErrorMsg(const Twine );
-  void printError(const Twine );
+  using Color = raw_ostream::Color;
+
+  std::string getLocation(const Twine );
 };
 
 /// Returns the default error handler.
Index: lld/trunk/test/ELF/color-diagnostics.test
===
--- lld/trunk/test/ELF/color-diagnostics.test
+++ lld/trunk/test/ELF/color-diagnostics.test
@@ -6,8 +6,8 @@
 # RUN: not ld.lld -xyz -color-diagnostics=always /nosuchfile 2>&1 \
 # RUN:   | FileCheck -check-prefix=COLOR %s
 
-# COLOR: {{ld.lld: .\[0;1;31merror: .\[0munknown argument '-xyz'}}
-# COLOR: {{ld.lld: .\[0;1;31merror: .\[0mcannot open /nosuchfile}}
+# COLOR: {{ld.lld: .\[0;31merror: .\[0munknown argument '-xyz'}}
+# COLOR: {{ld.lld: .\[0;31merror: .\[0mcannot open /nosuchfile}}
 
 # RUN: not ld.lld -color-diagnostics=foobar 2>&1 | FileCheck -check-prefix=ERR %s
 # ERR: unknown option: --color-diagnostics=foobar
Index: lld/trunk/test/COFF/color-diagnostics.test
===
--- lld/trunk/test/COFF/color-diagnostics.test
+++ lld/trunk/test/COFF/color-diagnostics.test
@@ -6,8 +6,8 @@
 # RUN: not lld-link -xyz --color-diagnostics=always /nosuchfile 2>&1 \
 # RUN:   | FileCheck -check-prefix=COLOR %s
 
-# COLOR: {{lld-link: .\[0;1;35mwarning: .\[0mignoring unknown argument '-xyz'}}
-# COLOR: {{lld-link: .\[0;1;31merror: .\[0mcould not open '/nosuchfile'}}
+# COLOR: {{lld-link: .\[0;35mwarning: .\[0mignoring unknown argument '-xyz'}}
+# COLOR: {{lld-link: .\[0;31merror: .\[0mcould not open '/nosuchfile'}}
 
 # RUN: not lld-link /nosuchfile 2>&1 | FileCheck -check-prefix=NOCOLOR %s
 # RUN: not lld-link -color-diagnostics=never /nosuchfile 2>&1 \
Index: lld/trunk/COFF/Driver.cpp
===
--- lld/trunk/COFF/Driver.cpp
+++ lld/trunk/COFF/Driver.cpp
@@ -62,7 +62,6 @@
 bool link(ArrayRef args, bool canExitEarly, raw_ostream ) {
   errorHandler().logName = args::getFilenameWithoutExe(args[0]);
   errorHandler().errorOS = 
-  errorHandler().colorDiagnostics = diag.has_colors();
   errorHandler().errorLimitExceededMsg =
   "too many errors emitted, stopping now"
   " (use /errorlimit:0 to see all errors)";
Index: lld/trunk/COFF/DriverUtils.cpp
===
--- lld/trunk/COFF/DriverUtils.cpp
+++ lld/trunk/COFF/DriverUtils.cpp
@@ -756,16 +756,17 @@
   OPT_no_color_diagnostics);
   if (!arg)
 return;
+
   if (arg->getOption().getID() == OPT_color_diagnostics) {
-errorHandler().colorDiagnostics = true;
+errorHandler().errorOS->enable_colors();
   } else if (arg->getOption().getID() == 

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

[Rui Ueyama via Phabricator via cfe-commits]

ruiu marked an inline comment as done.
ruiu added inline comments.



Comment at: clang/tools/diagtool/TreeView.cpp:30
 
-  TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(Color, false, false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
+  TreePrinter(llvm::raw_ostream ) : out(out), Internal(false) {
+if ( != ::errs() &&  != ::outs())

grimar wrote:
> nit: Seems `out` should be uppercase here.
> (I see it was like that before your changes, but seems you touching all the 
> places where it was used, so seems you can fix it).
Yes, this is inconsistent, but we probably shouldn't change this in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65564



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

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

[Rui Ueyama via Phabricator via cfe-commits]

ruiu updated this revision to Diff 212959.
ruiu added a comment.

- rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65564

Files:
  clang/include/clang/AST/ASTDumperUtils.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/tools/diagtool/TreeView.cpp
  lld/COFF/Driver.cpp
  lld/COFF/DriverUtils.cpp
  lld/Common/ErrorHandler.cpp
  lld/ELF/Driver.cpp
  lld/ELF/DriverUtils.cpp
  lld/include/lld/Common/ErrorHandler.h
  lld/lib/Driver/DarwinLdDriver.cpp
  lld/test/COFF/color-diagnostics.test
  lld/test/ELF/color-diagnostics.test
  lld/wasm/Driver.cpp
  llvm/include/llvm/Support/FormattedStream.h
  llvm/include/llvm/Support/WithColor.h
  llvm/include/llvm/Support/raw_ostream.h
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Support/raw_ostream.cpp
  llvm/tools/llvm-cov/CoverageReport.cpp
  llvm/tools/llvm-cov/CoverageViewOptions.h
  llvm/tools/llvm-cov/RenderingSupport.h
  llvm/tools/llvm-cov/SourceCoverageViewText.cpp
  llvm/tools/llvm-cov/llvm-cov.cpp
  llvm/tools/llvm-mca/Views/TimelineView.cpp
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -138,12 +138,11 @@
   /// The starting char (before tildes) for marking the line.
   char Lead;
   /// What color to use for this annotation.
-  raw_ostream::Colors Color;
+  raw_ostream::Color Color;
   /// A note to follow the marker, or empty string if none.
   std::string Note;
   MarkerStyle() {}
-  MarkerStyle(char Lead, raw_ostream::Colors Color,
-  const std::string  = "")
+  MarkerStyle(char Lead, raw_ostream::Color Color, const std::string  = "")
   : Lead(Lead), Color(Color), Note(Note) {}
 };
 
Index: llvm/tools/llvm-mca/Views/TimelineView.cpp
===
--- llvm/tools/llvm-mca/Views/TimelineView.cpp
+++ llvm/tools/llvm-mca/Views/TimelineView.cpp
@@ -103,8 +103,8 @@
 LastCycle = std::max(LastCycle, CurrentCycle);
 }
 
-static raw_ostream::Colors chooseColor(unsigned CumulativeCycles,
-   unsigned Executions, int BufferSize) {
+static raw_ostream::Color chooseColor(unsigned CumulativeCycles,
+  unsigned Executions, int BufferSize) {
   if (CumulativeCycles && BufferSize < 0)
 return raw_ostream::MAGENTA;
   unsigned Size = static_cast(BufferSize);
@@ -120,7 +120,7 @@
   if (!OS.has_colors())
 return;
 
-  raw_ostream::Colors Color = chooseColor(Cycles, Executions, BufferSize);
+  raw_ostream::Color Color = chooseColor(Cycles, Executions, BufferSize);
   if (Color == raw_ostream::SAVEDCOLOR) {
 OS.resetColor();
 return;
Index: llvm/tools/llvm-cov/llvm-cov.cpp
===
--- llvm/tools/llvm-cov/llvm-cov.cpp
+++ llvm/tools/llvm-cov/llvm-cov.cpp
@@ -83,13 +83,10 @@
 }
   }
 
-  if (argc > 1) {
-if (sys::Process::StandardErrHasColors())
-  errs().changeColor(raw_ostream::RED);
-errs() << "Unrecognized command: " << argv[1] << ".\n\n";
-if (sys::Process::StandardErrHasColors())
-  errs().resetColor();
-  }
+  if (argc > 1)
+errs() << raw_ostream::RED << "Unrecognized command: " << argv[1] << ".\n\n"
+   << raw_ostream::RESET;
+
   helpMain(argc, argv);
   return 1;
 }
Index: llvm/tools/llvm-cov/SourceCoverageViewText.cpp
===
--- llvm/tools/llvm-cov/SourceCoverageViewText.cpp
+++ llvm/tools/llvm-cov/SourceCoverageViewText.cpp
@@ -101,7 +101,7 @@
   auto *WrappedSegment = LCS.getWrappedSegment();
   CoverageSegmentArray Segments = LCS.getLineSegments();
 
-  Optional Highlight;
+  Optional Highlight;
   SmallVector, 2> HighlightedRanges;
 
   // The first segment overlaps from a previous line, so we treat it specially.
Index: llvm/tools/llvm-cov/RenderingSupport.h
===
--- llvm/tools/llvm-cov/RenderingSupport.h
+++ llvm/tools/llvm-cov/RenderingSupport.h
@@ -47,7 +47,7 @@
 /// Change the color of the output stream if the `IsColorUsed` flag
 /// is true. Returns an object that resets the color when destroyed.
 inline ColoredRawOstream colored_ostream(raw_ostream ,
- raw_ostream::Colors Color,
+ raw_ostream::Color Color,
  bool IsColorUsed = true,
  bool Bold = false, bool BG = false) {
   if (IsColorUsed)
Index: llvm/tools/llvm-cov/CoverageViewOptions.h
===
--- llvm/tools/llvm-cov/CoverageViewOptions.h
+++ llvm/tools/llvm-cov/CoverageViewOptions.h
@@ -46,7 +46,7 @@
 
   /// Change 

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

[Rui Ueyama via Phabricator via cfe-commits]

ruiu created this revision.
ruiu added reviewers: MaskRay, grimar.
Herald added subscribers: cfe-commits, thopre, gbedwell, aheejin, hiraditya, 
arichardson, sbc100, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: andreadb.
Herald added projects: clang, LLVM.

1. raw_ostream supports ANSI colors so that you can write messages

to the termina with colors. Previously, in order to change and reset
color, you had to call `changeColor` and `resetColor` functions,
respectively.

So, if you print out "error: " in red, for example, you had to do
something like this:

  OS.changeColor(raw_ostream::RED);
  OS << "error: ";
  OS.resetColor();

With this patch, you can write the same code as follows:

  OS << raw_ostream::RED << "error: " << raw_ostream::RESET;

2. Add a boolean flag to raw_ostream so that you can disable colored

output. If you disable colors, changeColor, operator<<(Color), resetColor
and other color-related functions have no effect.

Most LLVM tools automatically prints out messages using colors, and
you can disable it by passing a flag such as `--disable-colors`.
This new flag makes it easy to write code that works that way.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65564

Files:
  clang/include/clang/AST/ASTDumperUtils.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/tools/diagtool/TreeView.cpp
  lld/COFF/Driver.cpp
  lld/COFF/DriverUtils.cpp
  lld/Common/ErrorHandler.cpp
  lld/ELF/Driver.cpp
  lld/ELF/DriverUtils.cpp
  lld/include/lld/Common/ErrorHandler.h
  lld/lib/Driver/DarwinLdDriver.cpp
  lld/test/COFF/color-diagnostics.test
  lld/test/ELF/color-diagnostics.test
  lld/wasm/Driver.cpp
  llvm/include/llvm/Support/FormattedStream.h
  llvm/include/llvm/Support/WithColor.h
  llvm/include/llvm/Support/raw_ostream.h
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Support/raw_ostream.cpp
  llvm/tools/llvm-cov/CoverageReport.cpp
  llvm/tools/llvm-cov/CoverageViewOptions.h
  llvm/tools/llvm-cov/RenderingSupport.h
  llvm/tools/llvm-cov/SourceCoverageViewText.cpp
  llvm/tools/llvm-cov/llvm-cov.cpp
  llvm/tools/llvm-mca/Views/TimelineView.cpp
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -138,12 +138,11 @@
   /// The starting char (before tildes) for marking the line.
   char Lead;
   /// What color to use for this annotation.
-  raw_ostream::Colors Color;
+  raw_ostream::Color Color;
   /// A note to follow the marker, or empty string if none.
   std::string Note;
   MarkerStyle() {}
-  MarkerStyle(char Lead, raw_ostream::Colors Color,
-  const std::string  = "")
+  MarkerStyle(char Lead, raw_ostream::Color Color, const std::string  = "")
   : Lead(Lead), Color(Color), Note(Note) {}
 };
 
Index: llvm/tools/llvm-mca/Views/TimelineView.cpp
===
--- llvm/tools/llvm-mca/Views/TimelineView.cpp
+++ llvm/tools/llvm-mca/Views/TimelineView.cpp
@@ -103,8 +103,8 @@
 LastCycle = std::max(LastCycle, CurrentCycle);
 }
 
-static raw_ostream::Colors chooseColor(unsigned CumulativeCycles,
-   unsigned Executions, int BufferSize) {
+static raw_ostream::Color chooseColor(unsigned CumulativeCycles,
+  unsigned Executions, int BufferSize) {
   if (CumulativeCycles && BufferSize < 0)
 return raw_ostream::MAGENTA;
   unsigned Size = static_cast(BufferSize);
@@ -120,7 +120,7 @@
   if (!OS.has_colors())
 return;
 
-  raw_ostream::Colors Color = chooseColor(Cycles, Executions, BufferSize);
+  raw_ostream::Color Color = chooseColor(Cycles, Executions, BufferSize);
   if (Color == raw_ostream::SAVEDCOLOR) {
 OS.resetColor();
 return;
Index: llvm/tools/llvm-cov/llvm-cov.cpp
===
--- llvm/tools/llvm-cov/llvm-cov.cpp
+++ llvm/tools/llvm-cov/llvm-cov.cpp
@@ -83,13 +83,10 @@
 }
   }
 
-  if (argc > 1) {
-if (sys::Process::StandardErrHasColors())
-  errs().changeColor(raw_ostream::RED);
-errs() << "Unrecognized command: " << argv[1] << ".\n\n";
-if (sys::Process::StandardErrHasColors())
-  errs().resetColor();
-  }
+  if (argc > 1)
+errs() << raw_ostream::RED << "Unrecognized command: " << argv[1] << ".\n\n"
+   << raw_ostream::RESET;
+
   helpMain(argc, argv);
   return 1;
 }
Index: llvm/tools/llvm-cov/SourceCoverageViewText.cpp
===
--- llvm/tools/llvm-cov/SourceCoverageViewText.cpp
+++ llvm/tools/llvm-cov/SourceCoverageViewText.cpp
@@ -101,7 +101,7 @@
   auto *WrappedSegment = LCS.getWrappedSegment();
   CoverageSegmentArray Segments = LCS.getLineSegments();
 
-  Optional Highlight;
+  Optional Highlight;
   SmallVector, 2> HighlightedRanges;
 
   // The 

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu updated this revision to Diff 208883.
ruiu added a comment.

- Add a comment as to how to build and run clang-llvm-rename tool


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64123

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp

Index: clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
@@ -0,0 +1,287 @@
+//=== ClangLLVMRename.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This is a refactoring tool to rename variables so that they start with a
+// lowercase letter. This tool is intended to be used to rename variables in
+// LLVM codebase in which variable names start with an uppercase letter at
+// the moment.
+//
+//   Usage:
+//   clang-llmv-rename   ...
+//
+//  ... specify the paths of files in the CMake source tree. This
+// path is looked up in the compile command database. Here is how to build
+// the tool and apply that to all files under "lld" directory.
+//
+//   $ git clone https://github.com/llvm/llvm-project.git
+//   $ mkdir llvm-project/build
+//   $ cd llvm-project/build
+//   $ cmake -GNinja -DCMAKE_BUILD_TYPE=Release \
+//   -DLLVM_ENABLE_PROJECTS='clang-tools-extra;lld' \
+//   -DCMAKE_EXPORT_COMPILE_COMMANDS=On \
+//   ../llvm
+//   $ ninja
+//   $ bin/clang-llvm-rename ../lld/**/*.{cpp,h}
+//
+// For each variable in given files, the tool first check whether the
+// variable's definition is in one of given files or not, and rename it if
+// and only if it can find a definition of the variable. If the tool cannot
+// modify a definition of a variable, it doesn't rename it, in order to keep
+// a program compiles.
+//
+// Note that this tool is not perfect; it doesn't resolve or even detect
+// name conflicts caused by renaming. You may need to rename variables
+// before using this tool so that your program is free from name conflicts
+// due to lowercase/uppercase renaming.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+using namespace llvm;
+
+namespace {
+class RenameCallback : public MatchFinder::MatchCallback {
+public:
+  RenameCallback(
+  std::map ,
+  ArrayRef Paths)
+  : FileToReplacements(FileToReplacements) {
+for (StringRef S : Paths)
+  InputFiles.insert(canonicalizePath(S));
+  }
+
+  // This function is called for each AST pattern matche.
+  void run(const MatchFinder::MatchResult ) override {
+SourceManager  = *Result.SourceManager;
+
+if (auto *D = Result.Nodes.getNodeAs("VarDecl")) {
+  if (isa(D))
+return;
+  if (isGlobalConst(D))
+return;
+  convert(SM, D->getLocation(), D->getName());
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("ParmVarDecl")) {
+  if (auto *Fn =
+  dyn_cast_or_null(D->getParentFunctionOrMethod()))
+if (Fn->isImplicit())
+  return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("FieldDecl")) {
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("DeclRefExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (isa(D->getDecl()) ||
+  isa(D->getDecl()))
+return;
+  if (auto *Decl = dyn_cast(D->getFoundDecl()))
+if (isGlobalConst(Decl))
+  return;
+  if (D->getDecl()->getName().empty())
+return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = 

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I wasn't aware that clang-tidy had a such feature. 
readability-identifier-naming rule doesn't seem to work for this purpose out of 
the box. That being said, in hindsight, maybe I should have written this as a 
patch to clang-tidy instead of a new tool (which should have saved my time!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64123



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

[PATCH] D64156: Make joined instances of JoinedOrSeparate flags point to the unaliased args, like all other arg types do

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D64156



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

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu updated this revision to Diff 208116.
ruiu added a comment.

- updated a few special mappings


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64123

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp

Index: clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
@@ -0,0 +1,277 @@
+//=== ClangLLVMRename.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This is a refactoring tool to rename variables so that they start with a
+// lowercase letter. This tool is intended to be used to rename variables in
+// LLVM codebase in which variable names start with an uppercase letter at
+// the moment.
+//
+//   Usage:
+//   clang-llmv-rename   ...
+//
+//  ... specify the paths of files in the CMake source tree. This
+// path is looked up in the compile command database.
+//
+//
+// For each variable in given files, the tool first check whether the
+// variable's definition is in one of given files or not, and rename it if
+// and only if it can find a definition of the variable. If the tool cannot
+// modify a definition of a variable, it doesn't rename it, in order to keep
+// a program compiles.
+//
+// Note that this tool is not perfect; it doesn't resolve or even detect
+// name conflicts caused by renaming. You may need to rename variables
+// before using this tool so that your program is free from name conflicts
+// due to lowercase/uppercase renaming.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+using namespace llvm;
+
+namespace {
+class RenameCallback : public MatchFinder::MatchCallback {
+public:
+  RenameCallback(
+  std::map ,
+  ArrayRef Paths)
+  : FileToReplacements(FileToReplacements) {
+for (StringRef S : Paths)
+  InputFiles.insert(canonicalizePath(S));
+  }
+
+  // This function is called for each AST pattern matche.
+  void run(const MatchFinder::MatchResult ) override {
+SourceManager  = *Result.SourceManager;
+
+if (auto *D = Result.Nodes.getNodeAs("VarDecl")) {
+  if (isa(D))
+return;
+  if (isGlobalConst(D))
+return;
+  convert(SM, D->getLocation(), D->getName());
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("ParmVarDecl")) {
+  if (auto *Fn =
+  dyn_cast_or_null(D->getParentFunctionOrMethod()))
+if (Fn->isImplicit())
+  return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("FieldDecl")) {
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("DeclRefExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (isa(D->getDecl()) ||
+  isa(D->getDecl()))
+return;
+  if (auto *Decl = dyn_cast(D->getFoundDecl()))
+if (isGlobalConst(Decl))
+  return;
+  if (D->getDecl()->getName().empty())
+return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("MemberExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (D->getMemberDecl()->getName().empty())
+return;
+  convert(SM, D->getMemberLoc(), D->getMemberDecl()->getName());
+  return;
+}
+
+if (auto *D =
+Result.Nodes.getNodeAs("CXXCtorInitializer")) {
+  if (!isInGivenFiles(SM, D->getMemberLocation()))
+return;
+  convert(SM, D->getSourceLocation(), 

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D64123#1568096 , @Eugene.Zelenko 
wrote:

> There is clang-rename 
>  
> already. May be new functionality should be added there?


clang-rename seems to focus on renaming a variable in a selected region with 
the interactive use in mind. I.e. a user selects a variable on an IDE and 
rename its definition and all references.

I actually tried to create this tool based on clang-rename but failed because 
clang-rename was too slow for my purpose. It takes a non-negligible amount of 
time even for renaming a single variable, and I wanted to rename thousands or 
tens of thousands variables.

There are a few other significant differences between this tool and 
clang-rename as shown below:

- this tool renames a variable only when it can see the definition of the 
variable, while clang-rename renames a specified variable unconditionally
- clang-rename is an interactive tool, and you need to specify a new name by 
hand. On the other hand, this tool automatically generates a new name for each 
variable
- the core part of my tool is to find variables to rename, while clang-rename 
doesn't do anything for that

Overall, this tool is I think significantly different from clang-rename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64123



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

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu updated this revision to Diff 207937.
ruiu added a comment.

- removed a special rule for `E`
- do not lowercase global variables whose name is all uppercase
- OSec -> osec


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64123

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp

Index: clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
@@ -0,0 +1,275 @@
+//=== ClangLLVMRename.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This is a refactoring tool to rename variables so that they start with a
+// lowercase letter. This tool is intended to be used to rename variables in
+// LLVM codebase in which variable names start with an uppercase letter at
+// the moment.
+//
+//   Usage:
+//   clang-llmv-rename   ...
+//
+//  ... specify the paths of files in the CMake source tree. This
+// path is looked up in the compile command database.
+//
+//
+// For each variable in given files, the tool first check whether the
+// variable's definition is in one of given files or not, and rename it if
+// and only if it can find a definition of the variable. If the tool cannot
+// modify a definition of a variable, it doesn't rename it, in order to keep
+// a program compiles.
+//
+// Note that this tool is not perfect; it doesn't resolve or even detect
+// name conflicts caused by renaming. You may need to rename variables
+// before using this tool so that your program is free from name conflicts
+// due to lowercase/uppercase renaming.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+using namespace llvm;
+
+namespace {
+class RenameCallback : public MatchFinder::MatchCallback {
+public:
+  RenameCallback(
+  std::map ,
+  ArrayRef Paths)
+  : FileToReplacements(FileToReplacements) {
+for (StringRef S : Paths)
+  InputFiles.insert(canonicalizePath(S));
+  }
+
+  // This function is called for each AST pattern matche.
+  void run(const MatchFinder::MatchResult ) override {
+SourceManager  = *Result.SourceManager;
+
+if (auto *D = Result.Nodes.getNodeAs("VarDecl")) {
+  if (isa(D))
+return;
+  if (isGlobalConst(D))
+return;
+  convert(SM, D->getLocation(), D->getName());
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("ParmVarDecl")) {
+  if (auto *Fn =
+  dyn_cast_or_null(D->getParentFunctionOrMethod()))
+if (Fn->isImplicit())
+  return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("FieldDecl")) {
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("DeclRefExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (isa(D->getDecl()) ||
+  isa(D->getDecl()))
+return;
+  if (auto *Decl = dyn_cast(D->getFoundDecl()))
+if (isGlobalConst(Decl))
+  return;
+  if (D->getDecl()->getName().empty())
+return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("MemberExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (D->getMemberDecl()->getName().empty())
+return;
+  convert(SM, D->getMemberLoc(), D->getMemberDecl()->getName());
+  return;
+}
+
+if (auto *D =
+Result.Nodes.getNodeAs("CXXCtorInitializer")) {
+  if (!isInGivenFiles(SM, 

[PATCH] D64123: Add clang-llvm-rename tool.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu created this revision.
Herald added subscribers: cfe-commits, jfb, mgorny.
Herald added a project: clang.
ruiu planned changes to this revision.

Currently, this tool can rename variables in lld's source tree
without breaking it, but it is very unlikely that it will work
on any other LLVM subdirectories. I hand-tuned some renaming rules
to adopt it to lld's codebase.

This patch is not intended to be submitted, since this is a special-
purpose tool.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64123

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/CMakeLists.txt
  clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp

Index: clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-llvm-rename/ClangLLVMRename.cpp
@@ -0,0 +1,277 @@
+//=== ClangLLVMRename.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This is a refactoring tool to rename variables so that they start with a
+// lowercase letter. This tool is intended to be used to rename variables in
+// LLVM codebase in which variable names start with an uppercase letter at
+// the moment.
+//
+//   Usage:
+//   clang-llmv-rename   ...
+//
+//  ... specify the paths of files in the CMake source tree. This
+// path is looked up in the compile command database.
+//
+//
+// For each variable in given files, the tool first check whether the
+// variable's definition is in one of given files or not, and rename it if
+// and only if it can find a definition of the variable. If the tool cannot
+// modify a definition of a variable, it doesn't rename it, in order to keep
+// a program compiles.
+//
+// Note that this tool is not perfect; it doesn't resolve or even detect
+// name conflicts caused by renaming. You may need to rename variables
+// before using this tool so that your program is free from name conflicts
+// due to lowercase/uppercase renaming.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Execution.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+using namespace llvm;
+
+namespace {
+class RenameCallback : public MatchFinder::MatchCallback {
+public:
+  RenameCallback(
+  std::map ,
+  ArrayRef Paths)
+  : FileToReplacements(FileToReplacements) {
+for (StringRef S : Paths)
+  InputFiles.insert(canonicalizePath(S));
+  }
+
+  // This function is called for each AST pattern matche.
+  void run(const MatchFinder::MatchResult ) override {
+SourceManager  = *Result.SourceManager;
+
+if (auto *D = Result.Nodes.getNodeAs("VarDecl")) {
+  if (isa(D))
+return;
+  if (isConstant(D->getName()))
+return;
+  convert(SM, D->getLocation(), D->getName());
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("ParmVarDecl")) {
+  if (auto *Fn =
+  dyn_cast_or_null(D->getParentFunctionOrMethod()))
+if (Fn->isImplicit())
+  return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("FieldDecl")) {
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("DeclRefExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (isa(D->getDecl()) ||
+  isa(D->getDecl()))
+return;
+  if (isConstant(D->getDecl()->getName()))
+return;
+  if (D->getDecl()->getName().empty())
+return;
+  convert(SM, D->getLocation(), "");
+  return;
+}
+
+if (auto *D = Result.Nodes.getNodeAs("MemberExpr")) {
+  if (!isInGivenFiles(SM, D->getFoundDecl()->getLocation()))
+return;
+  if (D->getMemberDecl()->getName().empty())
+return;
+  convert(SM, D->getMemberLoc(), 

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I agree with Teresa. I don't think automatically setting O3 
 for Os and Oz is a good idea 
because these three options are different (that's why we have three different 
options in the first place). Adding an Os and Oz to lld's LTO option seems like 
a good idea, but they should be mapped to their corresponding features.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63976



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

[PATCH] D62523: Add release note entries for recent typo correction changes

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

LGTM


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D62523



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

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D60274



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

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I don't have any particular comment, and I'll give an LGTM soon as people seem 
to generally happy about this. If you are not happy about this patch or the 
feature itself, please shout. This feature is about to land.


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

https://reviews.llvm.org/D60274



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

[PATCH] D59765: [Lex] Warn about invisible Hangul whitespace

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I wonder if we should handle Unicode codepoints that are in the whitespace 
category as a whole, instead of handling each codepoint individually.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59765



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Overall looks good.

> Do you mean that of those three options, the last one specified should take 
> precedence?

Yes.




Comment at: ELF/Config.h:191
   bool ZGlobal;
+  GnuStackKind ZGnustack;
   bool ZHazardplt;

Members are (roughly) sorted alphabetically, so move this below the last 
boolean member.



Comment at: ELF/Driver.cpp:352
+  return GnuStackKind::Exec;
+else if (StringRef("noexecstack") == Arg->getValue())
+  return GnuStackKind::NoExec;

nit: no `else` after `return`.



Comment at: ELF/Driver.cpp:369
  S == "nodelete" || S == "nodlopen" || S == "noexecstack" ||
+ S == "nognustack" ||
  S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" 
||

If you have clang-format, please run it on this file.


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

https://reviews.llvm.org/D56554



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

If you still need to patch GNU ld, it doesn't seems like this patch makes 
things easier for you. (But even if this would make it easier for you, this 
patch's approach is not okay by design though.)


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

https://reviews.llvm.org/D56650



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

But that's only adding missing `-L` and perhaps a few more, no? That doesn't 
seem too hard to do in gcc. I don't want to repeat what compiler drivers do in 
the linker. Also, even with this patch, you need to make a change to gcc to 
pass `--target` parameter to the linker, right?


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

https://reviews.llvm.org/D56650



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Kamil,

I understand how adding `--target` option to the linker would make 
target-specific customization easier, but that's as I said not acceptable by 
design in lld. The code we have for FreeBSD just sets one bit in the output 
file, and that's fundamentally different from adding a "master switch" to turn 
on all customization for some specific target. We've been making changes to lld 
to make it work for many targets, but the features we've added are on by 
default (such as the code for Android TLS layout or supporting 
`.openbsd.randomdata` section for OpenBSD) or enabled by per-feature basis by a 
command line flag (such as *-freebsd emulation name). There's no flag like 
"turning lld into FreeBSD mode" or something like that. It seems to be working 
pretty well, and I'm happy about that design. So, I'm sorry, but I don't think 
I can support this patch's approach at all.

Honestly what's special about NetBSD from the lld's perspective seems to be the 
code ownership of the compiler driver. I can help you solve technical issues, 
but I don't think I should accept a patch that's against one of the lld's 
fundamental design choices to solve non-technical problems.


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

https://reviews.llvm.org/D56650



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

[PATCH] D56932: [Driver] [NetBSD] Pass default library search paths to linker

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Looks good but I'm probably not the right person to approve a change to this 
file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56932



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

No, I'm suggesting you add execstack, noexecstack and nognustack as a tri-state 
-z flag. Does this sound good?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56554#1357385 , @mgorny wrote:

> @ruiu, what do you think? The current logic forces some precedence, i.e. if 
> you pass `-z nognustack`, further `-z {no,}execstack` are ignored. I suppose 
> we could just force passing `-z nognustack` as last option clang-wise.


Are you asking about making the flag tri-state? If so, I think it logically 
makes sense and feels more natural to represent it with two boolean flags. As 
you said, the last one should always takes precedence over the others among the 
three flags.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

Let me make it clear again that I'm *not* okay with this approach. Please 
explicitly pass command line arguments from the compiler driver to the linker.


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

https://reviews.llvm.org/D56650



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56554#1353572 , @krytarowski wrote:

> What do you think about this new logic:
>
> 1. specified `-z execstack` -> do not emit GNU STACK segment
> 2. specified `-z noexecstack` -> emit GNU STACK segment
> 3. no option specified -> detect `findSection(".note.GNU-stack")` (I might 
> miss offhand some details here to be sure if it is reliable) 3.1. detected -> 
> emit GNU STACK segment 3.2. not detected -> do not emit GNU STACK segment
>
>   Additionally we will specify explicitly in the clang driver for Linux and 
> FreeBSD `-z noexecstack`.


I think I don't like the very idea of using "marker sections" in input object 
files to change the linker behavior. That's too subtle and seems error-prone to 
me.

After thinking for a while, I started thinking that the first version of this 
patch is probably exactly what we want. I had been thinking that `-z 
{no,}execstack` and `-z {no,}gnustack` are four different options, but what we 
actually want to get is tri-state:

- emit PT_GNU_STACK with RWX permission
- emit PT_GNU_STACK with RW permission
- do not emit PT_GNU_STACK

So we could map them to `-z {execstack,noexecstack,nognustack}`, respectively, 
with default set to `-z noexecstack`. You guys can pass `-z nognustack` to the 
linker to tell the linker to not emit it at all. That's exactly this patch does.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

GNU linkers assume that input object files that work with non-executable stack 
has a .note.GNU-stack section, and they emit a PT_GNU_STACK segment to mark the 
stack area non-executable if all input files have that marker section.

If restoring default means we implement that behavior, then lld shouldn't do 
that. That mechanism is error-prone, and the consequence of an error is 
disabling a major security measure. To be on the safe side, we simply emit 
unless it is explicitly to not do that by `-z execstack`.

If you add `-z nognustack` as an alias to `-z execstack` (and changing the 
behavior of `-z execstack` to not add the marker segment at all), I think I'm 
fine with that.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56554#1353527 , @krytarowski wrote:

> In D56554#1353513 , @ruiu wrote:
>
> > In D56554#1353487 , @krytarowski 
> > wrote:
> >
> > > In D56554#1353368 , @ruiu wrote:
> > >
> > > > The absence of PT_GNU_STACK segment makes stack area executable on 
> > > > systems that recognizes PT_GNU_STACK segment. So, I think if `-z 
> > > > execstack` is specified, we should omit PT_GNU_STACK segment rather 
> > > > than adding it, which I think you guys want. If we do that, it seems 
> > > > `-z nognustack` is a redundant option. That option name is unfortunate 
> > > > (you don't really mean you want an executable stack area), but that's I 
> > > > think still better than adding an option that is very similar to an 
> > > > existing feature.
> > >
> > >
> > > If we are going to change the meaning of `-z execstack`, can we rename 
> > > the option in lld? Probably to `-z gnustack` vs `-z nognustack`, probably 
> > > there is no other use-case than RWX->RW protection change.
> >
> >
> > Both `-z execstack` and `-z noexecstack` are supported by GNU linkers, so 
> > we can't simply rename them. We might be able to define aliases to the 
> > options. But I'm not sure if we want it if the only reason to do so is 
> > aesthetic reason.
>
>
> For compat with GNU linkers we could disable it by default, and emit GNU 
> stack only on demand.
>
> The current approach to enable GNU STACK is to inline a new segment 
> definition in assembly. It's done this way in llvm projects too.


Right. In terms of which is the default, lld is not compatible with GNU. lld by 
default tries to disable executable stack (which is definitely a good thing to 
do today). GNU linkers tries to do something smarter which doesn't make much 
sense and more error-prone today. I think changing the default makes sense.

Defining an alias to `-z execstack` as `-z nognustack` is a different story. 
I'm not totally against doing it, and I see that the option name is somewhat 
confusing, but I'm actually curious if the alias is what you really want. If 
you pass `-z noexecstack` to lld, your command line still works for both lld 
and GNU ld. But if you define `-z nognustack` and pass that option to lld, your 
command line is no longer compatible with GNU. You would no longer be able to 
copy all arguments to lld to run it again with GNU ld by just copy-n-pasting, 
for example. Are you happy with that?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56554#1353487 , @krytarowski wrote:

> In D56554#1353368 , @ruiu wrote:
>
> > The absence of PT_GNU_STACK segment makes stack area executable on systems 
> > that recognizes PT_GNU_STACK segment. So, I think if `-z execstack` is 
> > specified, we should omit PT_GNU_STACK segment rather than adding it, which 
> > I think you guys want. If we do that, it seems `-z nognustack` is a 
> > redundant option. That option name is unfortunate (you don't really mean 
> > you want an executable stack area), but that's I think still better than 
> > adding an option that is very similar to an existing feature.
>
>
> If we are going to change the meaning of `-z execstack`, can we rename the 
> option in lld? Probably to `-z gnustack` vs `-z nognustack`, probably there 
> is no other use-case than RWX->RW protection change.


Both `-z execstack` and `-z noexecstack` are supported by GNU linkers, so we 
can't simply rename them. We might be able to define aliases to the options. 
But I'm not sure if we want it if the only reason to do so is aesthetic reason.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

The absence of PT_GNU_STACK segment makes stack area executable on systems that 
recognizes PT_GNU_STACK segment. So, I think if `-z execstack` is specified, we 
should omit PT_GNU_STACK segment rather than adding it, which I think you guys 
want. If we do that, it seems `-z nognustack` is a redundant option. That 
option name is unfortunate (you don't really mean you want an executable stack 
area), but that's I think still better than adding an option that is very 
similar to an existing feature.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I want to handle NetBSD in the same way as the other operating systems. I'm 
sorry to have been saying no to a few recent patches for NetBSD, but I think 
that's for a good reason, and I don't think you presented a convincing reason 
why we had to handle only NetBSD differently.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

To be honest, I don't think I would lgtm a patch that changes lld's default 
behavior depending on host/target only for NetBSD, and it seems like a NetBSD's 
issue rather than an lld's issue. As I said, could you please make an effort to 
pass the flags you need from the compiler driver to the linker just like other 
systems do? That is easy to do, doesn't hurt anyone, probably a good thing to 
do  anyway as it makes options explicit rather than implicit, and solves at 
least most of the problems you have.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I'm sorry to say this but I don't think this is a good approach. Just like 
changing the default behavior depending on host hurts cross-build and is 
against the policy of the lld driver, changing the default behavior depending 
on the target hurts it too. Imagine that you are doing a cross-build on 
non-NetBSD system to create a NetBSD executable. You definitely want to ignore 
/usr/lib/i386 and /usr/lib on the host system. This patch does the opposite.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56288: [ELF] Do not enable 'new dtags' on NetBSD by default

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I think when we implemented the feature, there was an understanding that the 
"new dtags" might be "new" 20 years ago but they are not new at all now. This 
is not the only example of changing the default in lld; one example being that 
text segments are not writable by default (`-z text` is default as opposed to 
`-z notext`).

We could argue that making the text segment writable is a total nonsense today, 
but new/old dtags are different story. Bug even if that's the case, changing 
the default at this point has perhaps too much impact to existing lld users.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56288



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56215#1345880 , @krytarowski wrote:

> In D56215#1345845 , @ruiu wrote:
>
> > Not sure what I understand the point, but as to make lld work on/for 
> > NetBSD, I wonder if you can just add -L to the command line in the 
> > compiler driver. Isn't a NetBSD triple passed to the compiler driver? If 
> > so, I wonder if you can add a few extra options when invoking the linker.
>
>
> This describes the original patch of passing flags from compiler driver and 
> breaks GNU ld compat. Joerg expects to pass no extra `-L` or 
> `--disable-new-dtags` options, treating lld as a drop-in replacement for GNU 
> ld.


I don't think you would lose anything by passing `-L` and 
`--disable-new-dtags` from compiler driver. These flags wouldn't do any harm to 
GNU linkers.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Not sure what I understand the point, but as to make lld work on/for NetBSD, I 
wonder if you can just add -L to the command line in the compiler driver. 
Isn't a NetBSD triple passed to the compiler driver? If so, I wonder if you can 
add a few extra options when invoking the linker.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56288: [ELF] Do not enable 'new dtags' on NetBSD by default

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

The compiler driver should pass -disable-new-tags to the linker, but let's 
discuss that in https://reviews.llvm.org/D56215 instead of here.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56288



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56215#1345417 , @joerg wrote:

> Talking from the perspective of having had to deal with thousands of packages 
> in pkgsrc over the years: it is naive to believe that there isn't a lot of 
> software that calls the linker directly for various reasons. As such, it is 
> very important to have a useful configuration in the linker by default. Such 
> a configuration is by its very nature target specific. This doesn't mean it 
> can't be done in a cross-compiler friendly way, on the contrary. Over the 
> years NetBSD has been pushing its toolchain to be as similar for the native 
> build and a cross-target as reasonable possible. Modulo the build time 
> choices in the config.h sense, the only difference between the native and 
> cross tools is the built-in default of the former.


It might be naive but I don't think it's too naive. Most programs use ld via 
cc, and I don't think it is too unreasonable to not implement a host-specific 
logic for a very small percentage of programs that directly use ld. If we do, 
that logic would be the same or very similar to the one that we already have in 
cc. I think we should avoid that duplication of the host-specific config in the 
toolchain.

I see there are pros and cons to not have host-specific config in ld. That 
said, if other operating systems can live without host-specific config in lld, 
why can't NetBSD do? I really don't like to be in a situation where only one 
operating system have a host-specific config while others don't.


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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I sympathize and understand your frustration, but I don't know what I can do to 
you. The thing that lld doesn't have host-specific config is a policy matter 
we've been maintaining so far for all operating systems. I don't think NetBSD 
should be the only exception of the policy. In addition to that, I strongly 
believe the fact that lld's behavior only depends on the command line options 
given to the linker is easier to understand and generally a good thing.

Joerg, what is special about NetBSD?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

As you can see, lld doesn't really have any host-dependent knowledge so far, 
even though there are a few OSes that use lld as the default linker. We've 
added host-dependent knowledge to the compiler driver instead of to the linker 
for various operating systems. I guess that we could do the same thing for 
NetBSD, no?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56215#1344279 , @krytarowski wrote:

> In D56215#1344233 , @ruiu wrote:
>
> > lld's driver is intentionally designed to be agnostic of the host that the 
> > linker is running for the reasons described at the beginning of Driver.cpp: 
> > https://github.com/llvm-project/lld/blob/master/ELF/Driver.cpp#L13 I think 
> > I like that approach. If you need to do this, you can do this in the 
> > compiler driver rather than in the linker itself. Is there any reason you 
> > need to do this in the linker?
>
>
> This breaks compat with GNU ld here and the linker is intended to be used 
> standalone.


This is where lld is not 100% compatible with GNU ld, but I'd think that's not 
a bad thing. I'd like to make lld agnostics of host OS so that the linker works 
exactly in the same way on any operating systems, which makes cross linking 
much easier to do. So, both a run-time detection of a host OS or a 
configure-time customization are I think undesirable.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56215



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

[PATCH] D56215: [lld] [ELF] Include default search paths for NetBSD driver

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

lld's driver is intentionally designed to be agnostic of the host that the 
linker is running for the reasons described at the beginning of Driver.cpp: 
https://github.com/llvm-project/lld/blob/master/ELF/Driver.cpp#L13 I think I 
like that approach. If you need to do this, you can do this in the compiler 
driver rather than in the linker itself. Is there any reason you need to do 
this in the linker?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56215



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

[PATCH] D55443: [test] Capture stderr from 'tar --version' call as well

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D55443



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

[PATCH] D55443: [test] Capture stderr from 'tar --version' call as well

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: test/lit.cfg.py:100
 config.available_features.add('gnutar')
-tar_version.wait()

Maybe a silly question, but is this OK to remove this line?


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

https://reviews.llvm.org/D55443



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

[PATCH] D55443: [test] Capture stderr from 'tar --version' call as well

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: test/lit.cfg.py:99
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE,
+env={'LANG': 'C'})

ruiu wrote:
> mgorny wrote:
> > MaskRay wrote:
> > > If you don't need stderr, remove `stderr=subprocess.PIPE,`
> > > 
> > > `subprocess.Popen followed by communicate()` can be replaced by 
> > > `check_output`
> > > 
> > > `if 'GNU tar' in sout:` does not in Python 3 as `sout` would have type 
> > > `bytes`, not `str`
> > I need to silence stderr. Given that `/dev/null` is not portable, and 
> > `subprocess.DEVNULL` requires py3.3+, capturing it is the simplest solution.
> > 
> > `check_output()` will throw when `tar` doesn't support version, i.e. it 
> > would break NetBSD completely instead of silencing the error.
> > 
> > I'll fix the type mismatch.
> I think as Fangrui suggested
> 
>   subprocess.check_output(["/tmp/foo"], stderr=subprocess.STDOUT)
> 
> is better than subprocess.Popen followed by communicate() and wait().
Oh sorry I didn't know that `check_output` throws if a subprocess exits with 
non-zero exit code.


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

https://reviews.llvm.org/D55443



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

[PATCH] D55443: [test] Capture stderr from 'tar --version' call as well

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: test/lit.cfg.py:99
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE,
+env={'LANG': 'C'})

MaskRay wrote:
> If you don't need stderr, remove `stderr=subprocess.PIPE,`
> 
> `subprocess.Popen followed by communicate()` can be replaced by `check_output`
> 
> `if 'GNU tar' in sout:` does not in Python 3 as `sout` would have type 
> `bytes`, not `str`
I think as Fangrui suggested

  subprocess.check_output(["/tmp/foo"], stderr=subprocess.STDOUT)

is better than subprocess.Popen followed by communicate() and wait().


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

https://reviews.llvm.org/D55443



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

[PATCH] D55441: [lld] [test] Fix reproduce-blackslash.s test with NetBSD tar

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM

We might be able to change NetBSD's tar but I guess that takes very long time. 
This test is not very important in the sense that this tests just test a corner 
case. So I think we should just land this to make it work on NetBSD.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D55441



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

[PATCH] D55441: [lld] [test] Fix reproduce-blackslash.s test with NetBSD tar

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I thought NetBSD's tar is bsdtar because it's BSD... Anyways, I think I'm fine 
with this change, as the new test (which matches both `foo\\.o` and `foo\.o`) 
does not match a string that we don't want it to match.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D55441



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM. Please commit.

Peter, I wonder if you are fine with the default 64KiB page size with lld, 
especially given that lld always round up the text segment size to the maximum 
page size on disk and fill the padding with trap instructions. On average, that 
should increase the executable size by 32 KiB compared to other linkers. I 
don't think that size is necessarily bad, because we are doing that for a 
security purpose, but I wonder if people are OK with that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Yeah, I believe this patch is fine to submit, but since Peter is in a different 
timezone, I wanted give him a chance to review this one.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

I wouldn't rush to submit this now, given that this issue is not new at all. 
Maybe we can just wait for Peter's response?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

+peter.smith

Somewhat tangent to this patch, but is 64KiB a reasonable default for `-z 
max-page-size`? I believe that max-page-size is set to 64KiB so that OS/CPU 
whose minimum page size is 64KiB can load an executable linked with lld, but is 
there any real target OS/CPU that does not allow 4KiB pages? Or, is there any 
other reason to use that default value?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D53589: [bash-autocompletion] Fix bug when a flag ends with '='

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D53589



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

[PATCH] D51358: [driver] Do not pass "-flavor old-gnu" option to LLD linker

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D51358



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

[PATCH] D51234: [Driver] Change MipsLinux default linker from "lld" to "ld.lld"

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

If this piece of code used to be working correctly, there is another piece of 
code that adds `-flavor ld` to the command line. But if that's the case, this 
change wouldn't work because it constructs something like "ld.lld -flavor ld 
...". ld.lld doesn't accept `-flavor` option.

So my guess is this code is dead. Or, am I missing something?


Repository:
  rL LLVM

https://reviews.llvm.org/D51234



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

[PATCH] D50395: [WebAssembly] Remove use of lld -flavor flag

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D50395



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

[PATCH] D49899: Force test/Driver/fuchsia.c(pp) to use lld

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> The Fuchsia driver relies on lld so invoke clang with -fuse-ld=lld. This gets 
> the test passing when the clang default linker is something other than lld.

Does it work if lld is not installed at all? I believe if the driver cannot 
find a specified linker, it reports an error instead of trying to execute a 
nonexistent file.


https://reviews.llvm.org/D49899



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

Well, I don't think that pointing out that we shouldn't convert strings between 
UTF8 and UTF16 back and forth is nitpicking, but yeah, this has already been 
addressed, so feel free to submit. LGTM.


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: llvm/lib/Support/Windows/Process.inc:216
+  wchar_t ModuleName[MAX_PATH];
+  int Length = ::GetModuleFileNameW(NULL, ModuleName, MAX_PATH);
+  if (Length == 0 || Length == MAX_PATH) {

Can't this be size_t?


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: llvm/lib/Support/Windows/Process.inc:227
 return mapWindowsError(GetLastError());
-  if (Length > LongPath.capacity()) {
+  if (static_cast(Length) > MAX_PATH) {
 // We're not going to try to deal with paths longer than MAX_PATH, so we'll

and eliminate this static_cast.



Comment at: llvm/lib/Support/Windows/Process.inc:234
+
+  std::error_code ec = windows::UTF16ToUTF8(ModuleName, Length, Filename);
+  if (ec)

ec -> EC



Comment at: llvm/lib/Support/Windows/Process.inc:252
 
-  Args.reserve(ArgCount);
   std::error_code ec;
 

EC



Comment at: llvm/lib/Support/Windows/Process.inc:256
 
-  for (int i = 1; i < ArgCount && !ec; ++i) {
+  for (int i = 0; i < ArgCount; ++i) {
 ec = WildcardExpand(UnicodeCommandLine[i], Args, Alloc);

I


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: llvm/lib/Support/Windows/Process.inc:240
+  Filename.assign(Base.begin(), Base.end());
+  return ec;
 }

The intention of the code is to return a success, so it is less confusing if 
you directly return a success (i.e. std::error_code())



Comment at: llvm/lib/Support/Windows/Process.inc:262
 
-  LocalFree(UnicodeCommandLine);
+  SmallVector Arg0(Args[0], Args[0] + strlen(Args[0])), 
Filename;
+  sys::path::remove_filename(Arg0);

Looks like defining a variable on at a time is more common than defining two 
variables in one line.


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

So the best practice is, when you get a UTF-16 string from an Windows API, you 
should convert it to UTF-8 as soon as you can so that you can always process 
strings as UTF-8. Likewise, you should always keep all string in UTF-8 in 
memory and convert them to UTF-16 just before passing them to an Windows API if 
necessary. If you find yourself writing UTF-8 ↔ UTF-16 conversion code too 
frequently, that's like a signal that you are not doing text processing in 
UTF-8. Basically you need to do coding conversion up to only once for each new 
string.


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

It seems you converted the same string back and forth between UTF-8 and UTF-16 
to call Windows functions and LLVM functions. That's not only a waste of time 
(which is not a big problem) but complicates the code.

I'd define `std::error GetExecutableName(std::string )` which returns a 
filename (fully expanded but not a fullpath) of the current executable in 
UTF-8. Then, add something like this at the *end* of 
`GetCommandLineArguments()`:

  SmallVector Arg0(Args[0], strlen(Args[0]);
  sys::path::remove_filename(Arg0);
  GetExecutableName(Filename);
  sys::path::append(Arg0, Filename);
  Args[0] = AllocateString(Arg0, Alloc);


https://reviews.llvm.org/D47578



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

[PATCH] D47669: [cmake] Support LLD for CLANG_ORDER_FILE

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D47669



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

It seems like you are trying a bit too hard to keep the original code which is 
not always a good idea to keep the clarity of the code.

So, IIUC, you this function:

- returns a full filename of the current executable. That can be written using 
GetModuleFileName and GetLongPathName

then you basically do the following to

  sys::path::remove_filename(Arg0);
  sys::path::append(Arg0, sys::path::filename(ExectuablePath));
  





Comment at: llvm/lib/Support/Windows/Process.inc:211
 
 static std::error_code ExpandShortFileName(const wchar_t *Arg,
+   SmallVectorImpl ) 
{

This function is used only by your new function, so it is probably better to 
inline it.



Comment at: llvm/lib/Support/Windows/Process.inc:226
+
+static std::error_code GetLongArgv0FullPath(const wchar_t *Argv0,
+SmallVectorImpl 
) {

You can always ignore Argv0, no? Since GetModuleFileName is always available, 
you probably should use it unconditionally, ignoring the original argv[0].



Comment at: llvm/lib/Support/Windows/Process.inc:251
+  // This may change argv0 like below,
+  // * clang -> clang.exe (just add extension)
+  // * CLANG_~1.EXE -> clang++.exe (extend shorname)

I believe GetModuleFileName always returns a path with an extension, though it 
might be 8.3 path.



Comment at: llvm/lib/Support/Windows/Process.inc:268-269
+
+  return windows::UTF8ToUTF16(StringRef(UTF8Argv0.data(), UTF8Argv0.size()),
+  LongArgv0);
 }

Don't convert utf-8 to utf-16 only to convert it back to utf-8 immediately 
after returning from this function.


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added inline comments.



Comment at: llvm/lib/Support/Windows/Process.inc:226
+
+static std::error_code GetLongArgv0Path(const wchar_t *Argv0,
+SmallVectorImpl ,

It looks like this function is a bit too complicated. I'd define a function 
that expands DOS-style 8.3 path and adds missing extension, and use that 
function.


https://reviews.llvm.org/D47578



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

[PATCH] D47578: Do not enforce absolute path argv0 in windows

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Looks like this patch contains two unrelated changes. Please separate your 
change from the lit config changes.


https://reviews.llvm.org/D47578



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

(I found that information at 
http://wiki.bash-hackers.org/scripting/bashchanges#quoting_expansions_substitutions_and_related)


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Looks like $'' is there since bash 2.0 which should be pretty safe to use in 
the script. So feel free to use $'' instead of $(echo ...) in this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Oh, I didn't know the existence of $''. Thank you for the suggestion! I wonder 
which version of bash started supporting that notation. Do you know if all 
recent versions of bash support it? Unfortunately `$'' bash` is very hard query 
to google...


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

How about

  sed -e "$(echo -e 's/\t.*//')"

?


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D47273: [bash-completion] Fix tab separation on macOS

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Even though Perl may be installed to 99.99% of machines that use this 
autocomplete script, using perl instead of sed is too much. If we could use 
perl, we'd have wrote this script entirely in perl in the first place. We 
shouldn't add a dependency to perl.

I wonder if you could replace \t with \0x09. At least it works on my machine 
which has GNU sed.


Repository:
  rC Clang

https://reviews.llvm.org/D47273



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

[PATCH] D45634: Use InitLLVM in clang as well.

[Rui Ueyama via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC330067: Use InitLLVM in clang as well. (authored by ruiu, 
committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D45634?vs=142462=142463#toc

Repository:
  rC Clang

https://reviews.llvm.org/D45634

Files:
  tools/clang-format/ClangFormat.cpp
  tools/driver/driver.cpp


Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -12,9 +12,9 @@
 //
 
//===--===//
 
+#include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Driver/Compilation.h"
-#include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/ToolChain.h"
@@ -34,9 +34,8 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
-#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Regex.h"
@@ -322,22 +321,12 @@
 }
 
 int main(int argc_, const char **argv_) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv_[0]);
-  llvm::PrettyStackTraceProgram X(argc_, argv_);
-  llvm::llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
+  llvm::InitLLVM X(argc_, argv_);
+  SmallVector argv(argv_, argv_ + argc_);
 
   if (llvm::sys::Process::FixupStandardFileDescriptors())
 return 1;
 
-  SmallVector argv;
-  llvm::SpecificBumpPtrAllocator ArgAllocator;
-  std::error_code EC = llvm::sys::Process::GetArgumentVector(
-  argv, llvm::makeArrayRef(argv_, argc_), ArgAllocator);
-  if (EC) {
-llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
-return 1;
-  }
-
   llvm::InitializeAllTargets();
   auto TargetAndMode = ToolChain::getTargetAndModeFromProgramName(argv[0]);
 
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -22,8 +22,8 @@
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Support/Signals.h"
 
 using namespace llvm;
 using clang::tooling::Replacements;
@@ -338,21 +338,13 @@
 }
 
 int main(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
-
-  SmallVector Args;
-  llvm::SpecificBumpPtrAllocator ArgAllocator;
-  std::error_code EC = llvm::sys::Process::GetArgumentVector(
-  Args, llvm::makeArrayRef(argv, argc), ArgAllocator);
-  if (EC) {
-llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
-  }
+  llvm::InitLLVM X(argc, argv);
 
   cl::HideUnrelatedOptions(ClangFormatCategory);
 
   cl::SetVersionPrinter(PrintVersion);
   cl::ParseCommandLineOptions(
-  Args.size(), [0],
+  argc, argv,
   "A tool to format C/C++/Java/JavaScript/Objective-C/Protobuf code.\n\n"
   "If no arguments are specified, it formats the code from standard 
input\n"
   "and writes the result to the standard output.\n"


Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -12,9 +12,9 @@
 //
 //===--===//
 
+#include "clang/Driver/Driver.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Driver/Compilation.h"
-#include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/ToolChain.h"
@@ -34,9 +34,8 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
-#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Regex.h"
@@ -322,22 +321,12 @@
 }
 
 int main(int argc_, const char **argv_) {
-  llvm::sys::PrintStackTraceOnErrorSignal(argv_[0]);
-  llvm::PrettyStackTraceProgram X(argc_, argv_);
-  llvm::llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
+  llvm::InitLLVM X(argc_, argv_);
+  SmallVector argv(argv_, argv_ + argc_);
 
   if (llvm::sys::Process::FixupStandardFileDescriptors())
 return 1;
 
-  SmallVector argv;
-  llvm::SpecificBumpPtrAllocator ArgAllocator;
-  std::error_code EC = llvm::sys::Process::GetArgumentVector(
-  argv, llvm::makeArrayRef(argv_, argc_), ArgAllocator);
-  if (EC) {
-

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

This change looks good to me, but I think we shouldn't check in an executable 
binary file. Can you create `ld.foo.exe` in the test? Since you don't actually 
run ld.foo.exe, creating that executable should be very easy.


https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> That's weird, because lots of lldb tests compile and link test binaries on 
> Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
> `.exe` is necessary?

Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of 
lld-link?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> I also wonder which is better `#pragma comment(lib, "m")` or `#pragma 
> comment(lib, "m")`.

Sorry, I meant `#pragma comment(lib, "m")` or `#pragma comment("lib", "m")`.


Repository:
  rC Clang

https://reviews.llvm.org/D42758



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

[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

`#pragma comment` is what Microsoft is using, but is everybody happy about that 
name? It isn't clear at least to me that what it actually means is linker 
directives.

I also wonder which is better `#pragma comment(lib, "m")` or `#pragma 
comment(lib, "m")`.


Repository:
  rC Clang

https://reviews.llvm.org/D42758



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

[PATCH] D40234: [AutoComplete] Use stronger sort predicate for autocomplete candidates to remove non-deterministic ordering

[Rui Ueyama via Phabricator via cfe-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM




Comment at: lib/Driver/Driver.cpp:1204
+return X < 0;
+  return A.compare(B) > 0; });
 

I believe that if you use clang-format, `});` will be put to a separate line 
like this.

  std::sort(SuggestedCompletions.begin(), SuggestedCompletions.end(),
[](StringRef A, StringRef B) {
  if (int X = A.compare_lower(B))
return X < 0;
  return A.compare(B) > 0;
});


https://reviews.llvm.org/D40234



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

[PATCH] D40234: [AutoComplete] Use stronger sort predicate for autocomplete candidates to remove non-deterministic ordering

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Perhaps, this is a bit more straightforward.

  if (int X = A.compare_lower(B))
return X < 0;
  return A.compare(B) < 0;


https://reviews.llvm.org/D40234



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

[PATCH] D40234: [AutoComplete] Stable sort autocomplete candidates to remove non-deterministic ordering

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

Maybe we should do case-insensitive string comparison first, and if two strings 
are considered the same, try again in case-sensitive manner? Otherwise, even 
though the output is now deterministic, the output order is still dependent on 
the order of input strings.


https://reviews.llvm.org/D40234



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