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

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/m

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

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

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

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

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

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

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@lis

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

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 mai

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

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-commi

[PATCH] D38290: Add a ld64.lld alias for the MACHO LLD target

ruiu added a comment. Is this for cross-compilation? If you invoke lld on macOS, it should act as ld64. macOS lld is not actively maintained, so I wouldn't make it available more widely. Repository: rL LLVM https://reviews.llvm.org/D38290 ___

[PATCH] D38290: Add a ld64.lld alias for the MACHO LLD target

ruiu added a comment. This patch virtually sets `ld64` the linker command name for macOS. I'd be a bit reluctant doing that, because `ld64` sounds like a too generic name to indicate "a linker for macOS". But that's probably okay because we don't have a better name. Can you get an LGTM from so

[PATCH] D39017: [CMake] Build Fuchsia toolchain as -O3

ruiu added a comment. Yes, specifically, lld does string tail merging (instead of regular string merging) when -O2 is given, so it would produce slightly smaller outputs. Repository: rL LLVM https://reviews.llvm.org/D39017 ___ cfe-commits mailin

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

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` op

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

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/ma

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

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

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

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? Reposit

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

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 __

[PATCH] D33726: [driver][netbsd] Build and pass `-L` arguments to the linker

ruiu added a comment. I'm totally against adding per-OS path knowledge to our linker. Compilers already know include paths and I don't want to maintain another list of paths in the linker. Also this can be more confusing than useful when you are doing cross-linking. For all OSes other than Net

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu added a comment. Overall looking good. Good work! A few minor comments. Comment at: clang/include/clang/Driver/Options.td:2198 def stdlib_EQ : Joined<["-", "--"], "stdlib=">, Flags<[CC1Option]>, - HelpText<"C++ standard library to use">; + HelpText<"C++ standard library

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu added a comment. As to the order of Command variable contents, I think I'm not convinced. You can access the last element as `Command[Command.size() - 1]` (or maybe `Command.back()`), no? It is slightly awkward than `Command[0]`, but that's not horribly hard to handle, and passing argument

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu added inline comments. Comment at: clang/utils/bash-autocomplete.sh:10 + do +arg="$arg""${COMP_WORDS[$i]}," + done ruiu wrote: > nit: you don't need double double-quotes. Instead, `"$arg${COMP_WORDS[$i]}," > should just work. On second thought, I thin

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu added a comment. Yuka, This is beautiful. Overall looking pretty good. Some minor stylistic comments. Comment at: clang/include/clang/Driver/Options.h:43 +#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM, \ + HELPTEXT, METAVAR,

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu accepted this revision. ruiu added a comment. LGTM https://reviews.llvm.org/D33383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33383: [GSoC] Flag value completion for clang

ruiu added inline comments. Comment at: clang/include/clang/Driver/Options.td:496 def cl_std_EQ : Joined<["-"], "cl-std=">, Group, Flags<[CC1Option]>, - HelpText<"OpenCL language standard to compile for.">; + HelpText<"OpenCL language standard to compile for.">, Values<"cl,C

[PATCH] D34557: Sort the autocomplete candidates before printing them out.

ruiu created this revision. Currently, autocompleted options are displayed in the same order as we wrote them in .td files. This patch sort them out in clang so that they are sorted alphabetically. This should improve usability. https://reviews.llvm.org/D34557 Files: clang/lib/Driver/Driver.c

[PATCH] D34557: Sort the autocomplete candidates before printing them out.

This revision was automatically updated to reflect the committed changes. Closed by commit rL306116: Sort the autocomplete candidates before printing them out. (authored by ruiu). Changed prior to commit: https://reviews.llvm.org/D34557?vs=103731&id=103733#toc Repository: rL LLVM https://re

[PATCH] D34607: [Bash-autocompletion] Check clang version in Bash

ruiu added inline comments. Comment at: clang/utils/bash-autocomplete.sh:28 + flags=$( clang --autocomplete="$arg" 2>/dev/null ) + # Check if --autocomplete is supported in user's clang version. + if [[ "$?" != 0 ]]; then It is probably a bit better if you men

[PATCH] D34607: [Bash-autocompletion] Check clang version in Bash

ruiu accepted this revision. ruiu added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D34607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

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

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-b

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

ruiu added inline comments. Comment at: llvm/lib/Support/Windows/Process.inc:226 + +static std::error_code GetLongArgv0Path(const wchar_t *Argv0, +SmallVectorImpl &Args, It looks like this function is a bit too complicated.

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

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 GetLong

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

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

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 &Result)

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

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 befo

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

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::

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

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

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

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

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

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

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

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-commi

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

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 -> d

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

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://review

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

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 nognus

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

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 _

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

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 maili

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

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

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

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,

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

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://

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

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

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

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 __

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

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 ___

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

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 ave

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

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

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

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

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

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

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

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,

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

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/ ht

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

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 ___

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

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/D60

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

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

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

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) sort

[PATCH] D37203: [Bash-autocompletion] Follow up patch for D36782

ruiu added a comment. Technically, this patch fixes your issue, but I don't think this is a good way of fixing it because `parse()` function now has an internal state which is not obvious to the user of the function. Can you review https://reviews.llvm.org/D37217? This is an alternative fix tha

[PATCH] D37943: [docs] add Windows examples to ThinLTO.rst

ruiu accepted this revision. ruiu added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D37943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

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

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 su

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

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(out), ShowColors(hasColors(out)), Internal(false) {} - - void setColor(llvm::raw_ostream::Colors Color) { -

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

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 cl

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

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/D65

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

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 inte

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

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 `hasCo

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

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:

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

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

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

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 subd

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

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://review

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

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 foc

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

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-extr

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

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

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

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 sav

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

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/

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

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/mailm

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

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

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

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, y

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

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 reason

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

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 tha

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

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 po

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

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

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

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

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

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 th

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

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 -

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

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 defau

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

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 ar

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

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 comp

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

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 different

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

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

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

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 seg

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

ruiu added a comment. In D56554#1353527 , @krytarowski wrote: > In D56554#1353513 , @ruiu wrote: > > > In D56554#1353487 , @krytarowski > > wrote: > > > > > In D56554#135336

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

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 tha

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

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&id=142463#toc Reposi

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

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).

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

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

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

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/Com

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

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 `ltoVe

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

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 accompl

[PATCH] D36209: [Bash-autocompletion] Add comment to test so that it is easier to fix

ruiu added a comment. I wonder if this test is actually fragile. How can you break this test by adding a new flag? Comment at: clang/test/Driver/autocomplete.c:3 +// If this test had broke due to adding/modifying flags or changing HelpText, +// please modify this file accordin

  1   2   >