[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

[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

[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] 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

[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

[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

[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

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

[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

[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

[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

[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] 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)

[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

[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

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

[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

[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] 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

[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

[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

[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

[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

[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

[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

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

[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

[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

[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

[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] 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

[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

[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), 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

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

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

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

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

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

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`

[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

[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

[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] D70702: Use InitLLVM to setup a pretty stack printer

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/

[PATCH] D70694: Use InitLLVM in clang-tidy

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

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

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

[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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[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

<    1   2