[PATCH] D20561: Warn when taking address of packed member

joerg added a comment. The last review left out the case of naturally aligned packed members, IIRC. I have to go over the warning list in NetBSD again, but I'm moderately sure it is not fixed. The better example is: struct __attribute__((__packed__)) bar { uint16_t x1; uint16_t

[PATCH] D20561: Warn when taking address of packed member

joerg added a comment. Yes, the project is interested on reducing the number of false positives. The example you gave is *not* a FP, but exactly the kind of situation the warning is supposed to trigger on. Repository: rL LLVM https://reviews.llvm.org/D20561

[PATCH] D20561: Warn when taking address of packed member

joerg added a comment. It is not true that all false positives have been fixed, some of the more complex cases involving nested data structures are still open. @royger: Your example is missing explicit alignment. packed has two side effects: remove internal padding and set the alignment to 1.

[PATCH] D29630: [libcxx] Threading support: externalize sleep_for()

joerg added inline comments. Comment at: include/__threading_support:367 + ts.tv_sec = ts_sec_max; + ts.tv_nsec = giga::num - 1; + } I don't think giga::num makes things any clear compared to just spelling it out. Comment at:

[PATCH] D29630: [libcxx] Threading support: externalize sleep_for()

joerg accepted this revision. joerg added a comment. One small issue left, otherwise LGTM. Comment at: include/__threading_support:187 +_LIBCPP_THREAD_ABI_VISIBILITY +void __libcpp_thread_sleep_for(const chrono::nanoseconds& ns); + Drop the name here.

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

joerg added a comment. Generic should imply i486+. I don't think any general purpose system supports i386 at this point, simply because it has an annoying number of bugs in critical components. The i486 (esp. the non-crippled ones) are reasonable easy to support and there are still people

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

joerg added a comment. At this point, I don't think there is any use on pretending that i386-as-default makes sense. So I would request that the i386 case should be made explicit or just dropped, with a preference for the latter. https://reviews.llvm.org/D29542

[PATCH] D29032: [mips] Define macros related to -mabicalls in the preprocessor

joerg added a comment. I'm not terribly attached to using __ABICALLS__ for NetBSD, but let me check back with some of the MIPS folks. I would prefer __mips_abicalls to be always defined though, independent of the historic behavior. https://reviews.llvm.org/D29032

[PATCH] D29032: [mips] Define macros related to -mabicalls in the preprocessor

joerg added a comment. Always defining __mips_abicalls is certainly the saner choice. I'm discussing with the NetBSD/MIPS guys whether we just want to go with it all the time and drop our custom changes for __ABICALLS__. Chances are quite good that we are going in that direction.

[PATCH] D29843: [CodeGen] Treat auto-generated __dso_handle symbol as HiddenVisibility

joerg requested changes to this revision. joerg added inline comments. This revision now requires changes to proceed. Comment at: lib/CodeGen/CodeGenModule.cpp:2402 + return GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr, +

[PATCH] D28543: Elliminates uninitialized warning for volitile varibles.

joerg added a comment. Please fix the spelling errors in the titel / summary before commit. I somewhat agree with Hal -- I think this is too aggressive. Common use cases for local volatile include atomic ops or returns-twice functions like setjmp/longjmp. Disabling the warning in those cases

[PATCH] D30025: [compiler-rt] [builtins] Fix building atomic.c with GCC

joerg added a comment. Note that the normal compiler-rt functions have a different name than the builtins they provide, at least from the C frontend view. Repository: rL LLVM https://reviews.llvm.org/D30025 ___ cfe-commits mailing list

[PATCH] D29032: [mips] Define macros related to -mabicalls in the preprocessor

joerg added a comment. No need to preserve the BSD behavior for NetBSD. https://reviews.llvm.org/D29032 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg created this revision. joerg added a reviewer: klimek. joerg added a subscriber: cfe-commits. joerg set the repository for this revision to rL LLVM. In bigger projects like an Operating System, the same source code is often compiled in slightly different ways. This could be the difference

[PATCH] D27140: Allow clang to write compilation database records

joerg created this revision. joerg added reviewers: klimek, rsmith. joerg added a subscriber: cfe-commits. joerg set the repository for this revision to rL LLVM. joerg added a dependency: D27138: Extend CompilationDatabase by a field for the output filename. When integrating compilation database

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > Optional: I'd probably let the

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > joerg wrote: > > klimek wrote: > >

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > joerg wrote: > > klimek wrote: > >

[PATCH] D27140: Allow clang to write compilation database records

joerg removed rL LLVM as the repository for this revision. joerg updated this revision to Diff 79423. joerg added a comment. Use llvm::yaml::escape. https://reviews.llvm.org/D27140 Files: include/clang/Driver/Options.td lib/Driver/Tools.cpp Index: include/clang/Driver/Options.td

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added a comment. Can I interprete that as LGTM otherwise? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27140: Allow clang to write compilation database records

joerg updated this revision to Diff 79482. joerg added a comment. Move implementation into the Clang class, keep track of the raw_fd_stream. This avoids reopening it on multiple inputs and removes the need for append mode. Short circuit the function when -### is present. Add proper diagnostics

[PATCH] D27140: Allow clang to write compilation database records

This revision was automatically updated to reflect the committed changes. Closed by commit rL288821: Allow clang to write compilation database records. (authored by joerg). Changed prior to commit: https://reviews.llvm.org/D27140?vs=79997=80429#toc Repository: rL LLVM

[PATCH] D26564: Use PIC relocation mode by default for PowerPC64 ELF

This revision was automatically updated to reflect the committed changes. Closed by commit rL289744: Use PIC relocation mode by default for PowerPC64 ELF (authored by joerg). Changed prior to commit: https://reviews.llvm.org/D26564?vs=77670=81495#toc Repository: rL LLVM

[PATCH] D27304: [PATCH] [Sema][X86] Don't allow floating-point return types when SSE is disabled

joerg added a comment. I think this is the absolutely wrong place to put such logic. It really can not be anywhere but the backend. https://reviews.llvm.org/D27304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27304: [PATCH] [Sema][X86] Don't allow floating-point return types when SSE is disabled

joerg added a comment. The frontend is the wrong place as it doesn't even know if the register is ever going to be used. E.g. if it is a static function, all instances could be inlined away. https://reviews.llvm.org/D27304 ___ cfe-commits mailing

[PATCH] D27140: Allow clang to write compilation database records

joerg updated the summary for this revision. joerg set the repository for this revision to rL LLVM. joerg updated this revision to Diff 79997. joerg added a comment. Add test case. Repository: rL LLVM https://reviews.llvm.org/D27140 Files: include/clang/Basic/DiagnosticDriverKinds.td

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

This revision was automatically updated to reflect the committed changes. Closed by commit rL288436: Extend CompilationDatabase by a field for the output filename (authored by joerg). Changed prior to commit: https://reviews.llvm.org/D27138?vs=79320=79993#toc Repository: rL LLVM

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added a comment. It's not the directory, but the output file. That's optional since it is a new addition and I don't want to invalidate all existing JSON databases. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits

[PATCH] D27138: Extend CompilationDatabase by a field for the output filename

joerg added a comment. Which struct are we talking about, `CompileCommandRef` or `CompileCommand`? It is a pointer in the former and a plain StringRef in the latter. I don't think making it a pointer in both is an advantage, i.e. distinguishing empty input from missing field is not valuable in

[PATCH] D27304: [PATCH] [Sema][X86] Don't allow floating-point return types when SSE is disabled

joerg added a comment. Rejecting -mno-sse2 for x86_64 is even worse. Dynamic linkers e.g. in FreeBSD and NetBSD depend on that. They also don't contain floating point code, but that's a separate question. Similar constraints exist for the kernels. https://reviews.llvm.org/D27304

[PATCH] D27140: Allow clang to write compilation database records

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

[PATCH] D26564: Use PIC relocation mode by default for PowerPC64 ELF

joerg added a comment. Ping https://reviews.llvm.org/D26564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27066: Fix crash with unsupported architectures in Linux/Gnu target triples.

joerg accepted this revision. joerg added a reviewer: joerg. joerg added a comment. LGTM with a small cleanup. Comment at: lib/Driver/Tools.cpp:10018 + const char *LDMOption = getLDMOption(ToolChain.getTriple(), Args); + if (!LDMOption) { +

[PATCH] D28047: Remove the '-disable-llvm-passes' flag (which I didn't even know existed, and I suspect many others aren't aware of either) and strength '-disable-llvm-optzns' to do the same thing.

joerg added a comment. There are two issues I have with the flags right now. The first one you are addressing with this patch. The second issue is they remain CC1-only flags. Can we introduce a proper "-emit-raw-llvm" flag to work like "-emit-llvm" but without the additional IR processing. We

[PATCH] D31992: [clangd] Escape only necessary characters in JSON output

joerg added a comment. In https://reviews.llvm.org/D31992#725963, @klimek wrote: > If I understand correctly, that's " and \ for JSON and ", \ and all > non-printable characters (which unfortunately requires understanding of > unicode to solve this fully correctly) in YAML. I'd modify that

[PATCH] D31992: [clangd] Escape only necessary characters in JSON output

joerg added a comment. I'm strongly against this patch. Can you give an actual test case for the problematic behavior? Repository: rL LLVM https://reviews.llvm.org/D31992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31992: [clangd] Escape only necessary characters in JSON output

joerg added a comment. The short version is perfectly fine as long as works for both JSON and YAML. Less output is always good :) https://reviews.llvm.org/D31992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31992: [clangd] Escape only necessary characters in JSON output

joerg added a comment. Just to avoid any confusion: this should be the generic YAML escape routine in llvm/lib/Support, i.e. IMO we don't want to have separate YAML and JSON escape routines. Effective, change YAMLParser.cpp line 697 to use \u and drop the whole UTF-8 handling part.

[PATCH] D31992: [clangd] Escape only necessary characters in JSON output

joerg added a comment. We already have a couple of case that expect the encoding to be compatible. I'm not very attached to the additional special cases from YAML, but having either a common escape function OR a JSON escape in LLVM/Support is quite important. https://reviews.llvm.org/D31992

[PATCH] D35917: [mips] Implement -muninit-const-in-rodata

joerg added a comment. I don't see any reason why zero-initialised constants should be emitted in BSS. I know that GCC does that and I just fixed bugs in that because created wrong section flags for it. So yes, I'd prefer to revert this and fix the real problem. Repository: rL LLVM

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

joerg added a comment. In https://reviews.llvm.org/D34158#836026, @jyknight wrote: > In https://reviews.llvm.org/D34158#827178, @joerg wrote: > > > (2) It adds magic behavior that can make debugging more difficult. > > Partially preprocessed sources for example could be compiled with plain -c

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

joerg added a comment. In https://reviews.llvm.org/D34158#837444, @jyknight wrote: > Now, the "/tmp/foo-XXX.sh" also has a line labeled "Driver args: " with the > original command-line on it. If I understand correctly, you then like to take > this > simpler Driver command-line, and edit it

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

joerg added a comment. In https://reviews.llvm.org/D34158#837281, @hfinkel wrote: > In https://reviews.llvm.org/D34158#837130, @joerg wrote: > > > In https://reviews.llvm.org/D34158#836026, @jyknight wrote: > > > > > In https://reviews.llvm.org/D34158#827178, @joerg wrote: > > > > > > > (2) It

[PATCH] D35200: Don't use mmap on Windows

joerg added a comment. The primary reason for using mmap is not so much performance, but reduced memory foot print. https://reviews.llvm.org/D35200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35462: Also add the option -no-pie (like -nopie)

joerg accepted this revision. joerg added a comment. This revision is now accepted and ready to land. Please mark it as alias for -nopie, otherwise fine. https://reviews.llvm.org/D35462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

joerg added a comment. What if the constexpr function is called with in a non-constexpr context, e.g. with a random global variable as argument? https://reviews.llvm.org/D35190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

joerg added inline comments. Comment at: test/CodeGen/nouselookuptable.c:1 +// RUN: %clang_cc1 -S -fno-lookup-tables %s -emit-llvm -o - | FileCheck %s + Check positive flag and the default case as well. Comment at:

[PATCH] D35780: Introduce -nostdlib++ flag to disable linking the C++ standard library.

joerg added a comment. I don't really like this. The reason why -lm is added explicitly on many targets is because the C++ STL typically depends on it and that means for static linking and broken ELF linkers, it will be necessary to link against it explicitly. There is also the question on

[PATCH] D34926: Deprecation flag group + use for BB vectorizer options

joerg created this revision. Herald added a subscriber: rengolin. As discussed on IRC when https://reviews.llvm.org/D34846 came up, removing flags is problematic from a UX experience. This change introduces a new option group similar to the group GCC optimizer flags like

[PATCH] D34926: Deprecation flag group + use for BB vectorizer options

This revision was automatically updated to reflect the committed changes. Closed by commit rL306965: Add an option group for deprecated warnings. Add the removed (authored by joerg). Changed prior to commit: https://reviews.llvm.org/D34926?vs=104998=105004#toc Repository: rL LLVM

[PATCH] D34790: [NewPM] Add a flag -fexperimental-new-pass-manager=on/off/debug for printing debug output.

joerg added a comment. As I said in the discussion about similar flags for GVN, I'm generally fine with. We should have a big fat warning in the Release Notes that all -fexperimental-* flags are exactly that -- temporary and not intended for long term consumption. I.e. "we can and will remove

[PATCH] D32427: Fix float abi for SUSE ARM triples

joerg added a comment. I'm not sure the results will really work that well either. https://reviews.llvm.org/D32427 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34158: For standards compatibility, preinclude if the file is available

joerg added a comment. I had a long discussion with James about this on IRC without reaching a clear consensus. I consider forcing this behavior on all targets to be a major bug. It should be opt-in and opt-in only: (1) The header name is not mandated by any standard. It is not in any

[PATCH] D36764: The following functions and tests work fine for powerpc64, so enable them.

joerg added a comment. Because PPC uses the TC variant. https://reviews.llvm.org/D36764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33259: Don't defer to the GCC driver for linking arm-baremetal

joerg added a comment. While I fully agree that the current fallback-to-GCC behavior is far from helpful, I'm not sure how much sense providing linker support for bare-metal makes. I would expect this to changes wildly depending on the specific environment. https://reviews.llvm.org/D33259

[PATCH] D34506: Relax an assert in the comparison of source locations

joerg added a comment. I don't think it is a good idea to make this function non-transitive. Repository: rL LLVM https://reviews.llvm.org/D34506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.

joerg added a comment. I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess. Repository: rL LLVM https://reviews.llvm.org/D33304

[PATCH] D33272: Method loadFromCommandLine should be able to report errors

joerg added a comment. Can this use ErrorOr? https://reviews.llvm.org/D33272 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33356: [Nios2] Changes in frontend to support Nios2 LLVM target

joerg added inline comments. Comment at: lib/Basic/Targets.cpp:7614 + resetDataLayout(("e-" + Layout).str()); +} + } Can you just give the full string and avoid the runtime allocations? Comment at: lib/Basic/Targets.cpp:7650 +

[PATCH] D33356: [Nios2] Changes in frontend to support Nios2 LLVM target

joerg added inline comments. Comment at: lib/Basic/Targets.cpp:7650 +DefineStd(Builder, "nios2", Opts); +DefineStd(Builder, "NIOS2", Opts); + belickim wrote: > joerg wrote: > > DefineStd tends to give a lot of namespace pollution, is that intentional >

[PATCH] D33356: [Nios2] Changes in frontend to support Nios2 LLVM target

joerg added inline comments. Comment at: lib/Basic/Targets.cpp:7650 +DefineStd(Builder, "nios2", Opts); +DefineStd(Builder, "NIOS2", Opts); + belickim wrote: > joerg wrote: > > belickim wrote: > > > joerg wrote: > > > > DefineStd tends to give a lot of

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

joerg added a comment. A small subset can be found by searching for LINKER_RPATH_FLAG in pkgsrc. A classic offender is Emacs. For more, I would have to systematically search. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. Soft-float on the runtime environment part. I.e. non-trivial operations are lowered to library calls. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. At the very least, missing test case. I'm mostly ambivalent about this -- I don't really see the point and without matching soft float support it won't fully work either. Repository: rL LLVM https://reviews.llvm.org/D34018

[PATCH] D31972: Do not force the frame pointer by default for ARM EABI

joerg added a comment. Actually, for NetBSD we want to use -fomit-frame-pointer by default whenever the implicit -funwind-tables is also present. In general, that should be the justification for the default behavior: "Can I reliably unwind a binary with the available information?" Performance

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

joerg added a comment. In https://reviews.llvm.org/D33726#774105, @ruiu wrote: > 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

[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64

joerg added a comment. As I said, I don't see the point in pretending we support float128 when the runtime doesn't contain the necessary pieces. Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list

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

joerg added a comment. I'm against it. I consider this a strong bug on the LLD side and the behavior of clang correct. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

joerg added a comment. Such knowledge is necessary anyway. There is enough software that wants to use the linker directly. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

joerg added a comment. sysroot is already handled in NetBSD.cpp line 118 or so. Repository: rL LLVM https://reviews.llvm.org/D33726 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37302: [Headers] Define *_HAS_SUBNORM for FLT, DBL, LDBL

joerg added a comment. So what about targets that don't support subnormals? I'm moderately sure ARM falls into this category given the right phase of the moon. https://reviews.llvm.org/D37302 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. Well, the background for the use of the option in NetBSD is related to inducted differences in reproducable builds. From that perspective, it is even worth to sometimes shorten the dependency and sometimes not. https://reviews.llvm.org/D37954

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

joerg added a comment. ninja is not the only consumer of this. For human inspection, it can be quite surprising to see symlinks resolved, i.e. /usr/include/machine on NetBSD. Build systems have different ideas on whether they want absolute resolved paths or not, so it seems like ninja should

[PATCH] D37954: Expand absolute system header paths when using -MD depfiles

joerg added a comment. The comments at the very least are misleading. Resolving the path doesn't necessary shorten the string at all, in many cases it will be longer. I'm really not sure if clang should resolve symlinks like this as it can be quite surprising. https://reviews.llvm.org/D37954

[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early

joerg added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:2263 + } +} + vsk wrote: > rjmccall wrote: > > Is there a reason this only fails on x86? If LLVM doesn't have generic > > wide-operation lowering, this probably needs to be a target

[PATCH] D34158: For Linux/gnu compatibility, preinclude if the file is available

joerg added a comment. This version is fine with me. The only contentious part is whether it should be opt-in or opt-out for platforms, so getting this version in and revisiting the issue again later is OK from my perspective. https://reviews.llvm.org/D34158

[PATCH] D36764: The following functions and tests work fine for powerpc64, so enable them.

joerg added a comment. divtc3 and friends. https://reviews.llvm.org/D36764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

joerg added a comment. I've looked at this in some detail now. I'm not exactly sure yet why it is broken. The patch seems quite wrong to me. DW_CFA_GNU_args_size should be applied only when unwinding a call instruction and that regard, the commit message of the original change is quite

[PATCH] D41054: Teach clang/NetBSD about additional dependencies for sanitizers

joerg added a comment. I'm not really a fan of linking libutil into all binaries. Why is this code using forkpty in first place and not posix_openpt/grantpt? Repository: rL LLVM https://reviews.llvm.org/D41054 ___ cfe-commits mailing list

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

joerg added a comment. Let me phrase it differently. What is this patch (and the matching backend PR) supposed to achieve? There are effectively two ways to get rid of PLT entries: (1) Bind references locally. This is effectively what -Bsymbolic does and what is breaking the ELF interposition

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

joerg added a comment. Why again is this a good idea? This is an even worse hack than -Bsymbolic, the latter at least is visible in ELF header without code inspection. This is breaking core premises of ELF. https://reviews.llvm.org/D39079 ___

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

joerg added a comment. In https://reviews.llvm.org/D39079#905396, @rnk wrote: > In https://reviews.llvm.org/D39079#905372, @joerg wrote: > > > Why again is this a good idea? > > > It saves the direct jump to the PLT, reducing icache pressure, which is a > major cost in some workloads. It also

[PATCH] D39079: New clang option -fno-plt to avoid PLT for external calls

joerg added a comment. In https://reviews.llvm.org/D39079#905468, @rnk wrote: > In https://reviews.llvm.org/D39079#905454, @joerg wrote: > > > It also increases the pressure on the branch predictor, so it is not really > > black and white. > > > I don't understand this objection. I'm assuming

[PATCH] D47138: [Sparc] Use the leon arch for Leon3's when using an external assembler

joerg added a comment. This still needs a test case? https://reviews.llvm.org/D47138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

joerg updated this revision to Diff 150049. joerg added a comment. After a careful review of newer GCC / libgcc and the assembler annotations from LLVM, I have come to the following conclusions: (1) The semantics have been somewhat changed by GCC in recent years. There is no actual

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. This is not about any operating system, but basic consistent behavior. either do the canonicalisation or not. Doing it sometimes is just bogus. You've effectively implemented -fcanonical-system-headers=sometimes. Repository: rL LLVM https://reviews.llvm.org/D37954

[PATCH] D37954: Try to shorten system header paths when using -MD depfiles

joerg added a comment. The behavior of sometimes transforming the path and sometimes not is IMO completely unacceptable. I don't care if GCC created that bug first. Repository: rL LLVM https://reviews.llvm.org/D37954 ___ cfe-commits mailing

[PATCH] D39162: [test] Fix clang-test for FreeBSD and NetBSD

joerg added a comment. I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work. https://reviews.llvm.org/D39162 ___ cfe-commits

[PATCH] D42593: GCC compatibility: Ignore -fstack-clash-protection

joerg added a comment. I really don't like ignoring options that are supposed to provide actual functionality. Most of the other options are for pointless fine tuning and workarounds for broken gcc behavior in ancient versions. Repository: rC Clang https://reviews.llvm.org/D42593

[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables

joerg added a comment. There are two different considerations here: (1) Create less target code (2) Create less IR If this code can significantly reduce the amount of IR, it can be useful in general. That's why the existing memset logic is helpful. Repository: rL LLVM

[PATCH] D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types

joerg added a comment. I don't understand the desire for this logic. Why can't wasm override the rest of the types if it wants to have something special? Repository: rC Clang https://reviews.llvm.org/D50477 ___ cfe-commits mailing list

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

joerg added inline comments. Comment at: llvm/include/llvm/Support/YAMLTraits.h:477 + if (ParseOct && std::find(std::begin(OctalChars), std::end(OctalChars), +Char) == std::end(OctalChars)) +return false; Can you use

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

joerg added inline comments. Comment at: llvm/tools/llvm-yaml-numeric-parser-fuzzer/yaml-numeric-parser-fuzzer.cpp:16 + +llvm::Regex Inifnity("^[-+]?(\\.inf|\\.Inf|\\.INF)$"); +llvm::Regex Base8("^0o[0-7]+$"); Spelling? https://reviews.llvm.org/D50839

[PATCH] D50413: [libunwind][include] Add some missing definitions to .

joerg added a comment. Is there a reason for defining them? As in: does anything outside libunwind use them? I haven't seen such software yet. Repository: rUNW libunwind https://reviews.llvm.org/D50413 ___ cfe-commits mailing list

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

joerg added a comment. Why do we need to allocate memory in this case at all? I.e. why can't this just be: if (S.empty()) return StringRef("", 0); ... Repository: rL LLVM https://reviews.llvm.org/D50966 ___ cfe-commits mailing list

[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

joerg added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:609 static void addDebugPrefixMapArg(const Driver , const ArgList , ArgStringList ) { + for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) { +StringRef Map = A->getValue();

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

This revision was automatically updated to reflect the committed changes. Closed by commit rL337312: The semantics of DW_CFA_GNU_args_size have changed subtile over the (authored by joerg, committed by ). Herald added subscribers: llvm-commits, christof. Changed prior to commit:

[PATCH] D49482: Haiku: add a test for haiku driver

joerg added a comment. It seems to miss most of the interesting checks, i.e. crt files. Compare with any of the entries on netbsd.c for example. Repository: rC Clang https://reviews.llvm.org/D49482 ___ cfe-commits mailing list

[PATCH] D49480: Haiku: support for secondary arch

joerg requested changes to this revision. joerg added a comment. This revision now requires changes to proceed. This is absolutely not how the clang driver is supposed to work. No conditional compilation. Repository: rC Clang https://reviews.llvm.org/D49480

[PATCH] D49481: Haiku: Enable thread-local storage and disable PIE by default

joerg requested changes to this revision. joerg added a comment. This revision now requires changes to proceed. Both needs a test case :) Repository: rC Clang https://reviews.llvm.org/D49481 ___ cfe-commits mailing list

  1   2   3   >