[Lldb-commits] [PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

MaskRay requested changes to this revision. MaskRay added a comment. This revision now requires changes to proceed. Thanks for the patch, but there are two issues that should be fixed: (1) stat => fstat (2) change the subject to mean that this is not AIX specific (`[LLVM][Support] Report EISDIR

[Lldb-commits] [PATCH] D157497: feat: Migrate isArch16Bit

MaskRay added a comment. In D157497#4584621 , @Pivnoy wrote: > This discussion was the main motivation for this change. > https://discourse.llvm.org/t/rfc-refactor-triple-related-classes/70410/11 > As a result, Triple should become a data class, and its

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

MaskRay added inline comments. Comment at: lld/ELF/Driver.h:28 OPT_INVALID = 0, -#define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID, +#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__), #include "Options.inc" lld/wasm lld/COFF

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. In D157028#4558724 , @jansvoboda11 wrote: > Here's an example of a patch that changes the `OPTION` macro: D157029 >

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

MaskRay added a comment. I'll be away for a few days but I took a quick glance. This change looks reasonable. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157028/new/ https://reviews.llvm.org/D157028

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

MaskRay added inline comments. Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:23 + +class TestMCDisasmInstanceRISCV : public testing::Test { +public: Place all classes and test methods in an anonymous namespace.

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

MaskRay added a comment. It seems that a lldb specific test is needed. Adding a new method to `llvm/include/llvm/MC/MCInstrAnalysis.h` is fine with me, though I haven't checked the semantics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

MaskRay added a comment. Reverse ping @philnik :) The `release/17.x` branch will be created soon (https://discourse.llvm.org/t/llvm-17-0-0-release-planning-and-update/71762). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151683/new/

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

MaskRay added a comment. In D155178#4499105 , @MaskRay wrote: > In D155178#4498911 , @thakis wrote: > >> http://45.33.8.238/linux/112305/step_4.txt looks like a missing include > > I've fixed it in

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

MaskRay added subscribers: mysterymath, phosek. MaskRay added a comment. In D155178#4498911 , @thakis wrote: > http://45.33.8.238/linux/112305/step_4.txt looks like a missing include I've fixed it in 816141ce0eb899178dbcb6f0671875eb825b2f84

[Lldb-commits] [PATCH] D155018: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 5)

MaskRay added a comment. Still, would be good to land the lldb commit separately. And ensure that you have performed the last-minute testing... Ensure that projects like `flang bolt` still build. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

MaskRay added a comment. In D151683#4486321 , @aaron.ballman wrote: > Aside from the failing precommit CI test case in C, I think this LGTM. I've > added @MaskRay as the code owner for the command line option changes in case > he's got concerns

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

MaskRay added a comment. Hi! The added lines have some spaces at line ends. I think using `clang-format-diff.py` would remove them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154542/new/ https://reviews.llvm.org/D154542

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

MaskRay added a comment. In D151683#4409633 , @aaron.ballman wrote: > In D151683#4384017 , @erichkeane > wrote: > >> In D151683#4382408 , @philnik >> wrote: >> >>> No.

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

MaskRay added a comment. In D141907#4094748 , @MaskRay wrote: > [...] > edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable > `CLANG_RESOURCE_DIR` but did not explain why. > In the long term, the CMake variable `CLANG_RESOURCE_DIR`

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. If `nonMicrosoftDemangle` is going to be changed to take a `string_view` argument, adding `if (!MangledName) return false;` should be fine. If possible, it's best for the change to be

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

MaskRay added inline comments. Comment at: llvm/lib/Demangle/RustDemangle.cpp:150 -char *llvm::rustDemangle(const char *MangledName) { - if (MangledName == nullptr) -return nullptr; I see that this patch drops `if (MangledName == nullptr)` (D101444). This

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

MaskRay added inline comments. Comment at: llvm/lib/Demangle/Demangle.cpp:49 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) { + if (!MangledName) +return false; nickdesaulniers wrote: > nickdesaulniers wrote: > > MaskRay wrote: > >

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

MaskRay added inline comments. Comment at: llvm/lib/Demangle/Demangle.cpp:49 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) { + if (!MangledName) +return false; Why is this change? The original contract is that `MangledName` must be

[Lldb-commits] [PATCH] D150996: LLVM_FALLTHROUGH => [[fallthrough]]. NFC

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150996/new/ https://reviews.llvm.org/D150996

[Lldb-commits] [PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

MaskRay added a comment. Herald added a subscriber: JDevlieghere. Thank you for making another try for the treewide change (which is admittedly very painful and not many people do such work). Can you include the original patch description to the summary? That is the main part and the message

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

MaskRay accepted this revision. MaskRay added a subscriber: labath. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: lldb/include/lldb/Utility/ConstString.h:186 operator llvm::StringRef() const { return GetStringRef(); } + //

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

MaskRay added a comment. In D148384#4270505 , @dhoekwater wrote: > It looks like this breaks the build: > https://lab.llvm.org/buildbot#builders/119/builds/12865 > https://lab.llvm.org/buildbot#builders/123/builds/18388 >

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

MaskRay added inline comments. Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:19 +#include + chapuni wrote: > Odd layering... Good catch. LLVMDemangle cannot depend on other LLVM libraries. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

MaskRay added inline comments. Comment at: lldb/include/lldb/lldb-private-types.h:14 +#include "lldb/Target/RegisterFlags.h" #include "lldb/lldb-private.h" Is there a library layering violation? I assume that `lldb/Target/*.h` files depend on

[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

MaskRay added a comment. Herald added subscribers: bviyer, ekilmer, jplehr, thopre. @Ericson2314 @phosek What's the state of this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132608/new/ https://reviews.llvm.org/D132608

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

MaskRay added a comment. In D138539#4164313 , @steakhal wrote: > Oh, I forgot to mention why this change breaks the equality operator of > `llvm::StringSet`. It's because the `StringMap::operator==` will try to > compare the `value` as well, which is

[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

MaskRay added a comment. This seems to cause many lldb failures https://lab.llvm.org/buildbot/#/builders/68/builds/47790 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143652/new/ https://reviews.llvm.org/D143652

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/test/ObjectYAML/Offload/multiple_members.yaml:21 Value:"gfx908" Content: "cafefeed" Rebase:) I fixed obj2yaml Comment at:

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

MaskRay added a comment. > `For posterity, the tests that fail on main are:` > > `Skipped : 43` > [...] You can remove these from the description. They are flaky tests unrelated to this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: mlir/test/Transforms/print-op-graph.mlir:1 // RUN: mlir-opt -allow-unregistered-dialect -mlir-elide-elementsattrs-if-larger=2 -view-op-graph %s -o %t

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

MaskRay added inline comments. Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:638 - EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"))); + EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.h"), testPath("A.cc"))); // Make

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

MaskRay added a comment. edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable `CLANG_RESOURCE_DIR` but did not explain why. The patch introduced conditions in C++ code like std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

MaskRay added a comment. To enable smooth hash function migration in the future, we can explore the idea of adding `#ifdef EXPENSIVE_CHECKS\nshuffle` (see https://github.com/llvm/llvm-project/issues/34483). That means we should fix these tests properly to be non-dependent on the iteration

[Lldb-commits] [PATCH] D140999: [NFC][TargetParser] Deprecate llvm/Support/AArch64TargetParser.h

MaskRay accepted this revision. MaskRay added a comment. I don't think it is necessary to deprecate the old header then delete it after 16.0.0 is branched. llvm/Support/AArch64TargetParser.h has very few open-source out-of-tree uses. Perhaps only ldc `driver/targetmachine.cpp` uses the header.

[Lldb-commits] [PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

MaskRay accepted this revision. MaskRay added a comment. LGTM if `makeArrayRef` is kept in this patch. Do we foresee compiler bugs (for supported compilers) in this area? If there may be a risk, consider migrate a part of the code base first. CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

MaskRay added a comment. Can you push `using OptionalFileEntryRef = CustomizableOptional;` and the renaming as a separate commit to make this patch smaller? There is a smaller that this rename may be reverted. 205c0589f918f95d2f2c586a01bea2716d73d603

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay accepted this revision. MaskRay added a comment. Please check these builds all work: - default - `-DLLVM_LINK_LLVM_DYLIB=on` - `-DBUILD_SHARED_LIBS=on` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay added a comment. The name `llvm/lib/TargetParser` LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/ https://reviews.llvm.org/D137838 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay added a comment. Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit when more stuff of this kind was introduced into LLVMSupport. +1 for adding temporary forwarding headers for now to avoid update all `#include` users. Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D137724: [CMake] Warn when the version is older than 3.20.0.

MaskRay accepted this revision as: MaskRay. MaskRay added a comment. I think `if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)` checks for standalone builds is not necessary. The check in `llvm/CMakeLists.txt` suffices. It's unlikely the users will use different cmake versions to configure

[Lldb-commits] [PATCH] D137503: [CMake] Fix -Wstrict-prototypes

MaskRay added a comment. In D137503#3910651 , @aaron.ballman wrote: > LGTM, thank you for this! If we do a 15.0.5, I think these changes should > probably go into there as well (WDYT?) I support this. This will make llvm-project 15.0.5 buildable by

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

MaskRay added a comment. Herald added a subscriber: Moerafaat. > We held off on this before as `LLVM_LIBDIR_SUFFIX` conflicted with it. Now we > return this. > > I imagine this is too potentially-breaking to make LLVM 15. That's fine. ... These sentences are no longer relevant and should be

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. zstd provides GNU Makefile, CMake, and Meson. The CMake files are only installed when configured with CMake. Debian and Ubuntu lack such files. The pkg-config file libzstd.pc is probably the most portable file. (I have also used it for a binutils-gdb patch.) I think we

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; MaskRay wrote: > I missed the values here. Why is -5 picked for NoCompression? What does it >

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; I missed the values here. Why is -5 picked for NoCompression? What does it mean? zstd.h says

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. I am still seeing the zstdConfig.cmake zstd-config.cmake warning. @ckissane @phosek will you fix it? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Does this address the macOS build failure? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[Lldb-commits] [PATCH] D138310: [NFC] Make headers self-contained.

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Herald added a subscriber: StephenFan. Comment at: lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h:15 -#include "EmulateInstructionRISCV.h" #include

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

MaskRay added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:218 + explicit NameLookup(std::nullopt_t) : Data(nullptr, true) {} explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {} NameLookup() : NameLookup(nullptr) {}

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

MaskRay added a comment. In D137217#3945366 , @glandium wrote: > Almost there, but not quite: > > [task 2022-11-22T23:55:36.341Z] > /builds/worker/fetches/llvm-project/llvm/tools/gold/gold-plugin.cpp:1106:6: > error: no matching function for call to

[Lldb-commits] [PATCH] D138376: Use None consistently (NFC)

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. `enum class NoneType { None = 1 };` is from 0cd22f9540c0591132ec991c51103cf800cf4e24 (2017) for MSVC workaround. I assume it is no longer needed.. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D138276: TableGen: require tablegen cl::opts to be registered explicitly

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Herald added a subscriber: StephenFan. This is similar to `mlir::register*Options` and looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

MaskRay added inline comments. Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:846 + auto AddStream = [&](size_t Task, + Twine ModuleName) -> std::unique_ptr { int FD = -1; `Twine` should only be used as const

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

MaskRay added inline comments. Comment at: lld/COFF/LTO.cpp:229 +StringRef ltoObjName; +if (bitcodeFilePath == "ld-temp.o") { + ltoObjName = MaskRay wrote: > tejohnson wrote: > > zequanwu wrote: > > > tejohnson wrote: > > > > This case should always

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

MaskRay added a comment. In D137217#3926601 , @tejohnson wrote: > @MaskRay wondering if this is a good change to make for ELF as well, wdyt? Yes, I think this is a good idea and improves debuggability. The change is non-trivial so so this patch

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Considering https://discourse.llvm.org/t/should-we-continue-embed-the-full-llvm-version-in-lib-clang/62094 and this thread, I think overall people favor this patch. If a

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

MaskRay added a comment. It'll be good if we have a wiki describing how downstream users invoke cmake, so that large cmake refactoring can be verified beforehand. (like usage verification https://github.com/opencollab/llvm-toolchain-integration-test-suite, but for cmake invocations) CHANGES

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/Driver/debug-options-aranges.c:6 // RUN: %clang -### -g --target=x86_64-linux -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck %s -// CHECK:

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

MaskRay added a comment. I missed https://reviews.llvm.org/D134668 . See my comment there I don't think the change is necessary. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135400/new/ https://reviews.llvm.org/D135400 ___ lldb-commits

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

MaskRay added a comment. D134385 should fix the problem:) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133525/new/ https://reviews.llvm.org/D133525 ___ lldb-commits

[Lldb-commits] [PATCH] D132300: [clang][lldb][cmake] Use new `*_INSTALL_LIBDIR_BASENAME` CPP macro

MaskRay added a comment. In the long term we should just remove the `CLANG_INSTALL_LIBDIR_BASENAME` customization. This is supposed for GCC multilib lib32 lib64 names but we don't necessarily use it for Clang + compiler-rt files. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

MaskRay added a comment. Ping for unresolved issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133530/new/ https://reviews.llvm.org/D133530 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

MaskRay added inline comments. Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:509 + const ElfType OutputElfType = + getOutputElfType(Config.OutputArch.value_or(MachineInfo())); + const bool Is64Bit = This is incorrect. Config.OutputArch is usually unset

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

MaskRay added a comment. See `test/tools/llvm-objcopy/ELF/compress-debug-sections-zstd.test`. Use a similar file for ELFCLASS32 which runs `llvm-objcopy --compress-debug-sections=zstd` then `llvm-objcopy --decompress-debug-sections`. Then compare the output with the original with just one

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

MaskRay added inline comments. Comment at: lldb/test/Shell/ObjectFile/ELF/compressed-sections-zstd.yaml:19 +Content: deadbeefbaadf00d +## The legacy .zdebug format is not supported. + - Name:.zdebug_info Delete `.zdebug`. It is unrelated

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

MaskRay added a comment. In D111509#3778882 , @mizvekov wrote: > In D111509#3778879 , @MaskRay wrote: > >> Since the fix was not merged, I will revert this soon. >> >> I saw a clang segfault with `clang++

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

MaskRay added a comment. Since the fix was not merged, I will revert this soon. I saw a clang segfault with `clang++ libstdc++-v3/src/c++98/complex_io.cc` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111509/new/ https://reviews.llvm.org/D111509

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

MaskRay added a comment. In D130689#3710291 , @aaron.ballman wrote: > In D130689#3710281 , @royjacobson > wrote: > >> In D130689#3709834 , @thieta wrote: >> >>> In

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcdeb50c32155: [lldb] Remove include/lldb/lldb-private.h (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

MaskRay created this revision. MaskRay added reviewers: JDevlieghere, kastiglione. Herald added a subscriber: StephenFan. Herald added a project: All. MaskRay requested review of this revision. Herald added projects: clang, LLDB. Herald added subscribers: lldb-commits, cfe-commits. The header

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

MaskRay added a comment. In D130689#3704581 , @dyung wrote: > Your change is causing a build failure on the PS4 linux build bot using GCC > 9.3. Can you take a look? > https://lab.llvm.org/buildbot/#/builders/139/builds/26186 > > FAILED: >

[Lldb-commits] [PATCH] D128465: [OLD] [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. In D128465#3660733 , @ckissane wrote: > In D128465#3660714 , @MaskRay wrote: > >> Instead of marking a differential `[OLD] ` (which may confuse readers >> whether this is to be

[Lldb-commits] [PATCH] D128465: [OLD] [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. Instead of marking a differential `[OLD] ` (which may confuse readers whether this is to be abandoned), you can mark a differential `Request Reviews`/`Request Changes`. Then it will not be in the green state. I clicked "Reopen" to make it clear the patch hasn't

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

This revision was automatically updated to reflect the committed changes. MaskRay marked an inline comment as done. Closed by commit rGecfaf4801cd0: [lldb] Remove ELF .zdebug support (authored by MaskRay). Changed prior to commit: https://reviews.llvm.org/D129724?vs=444504=444707#toc

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

MaskRay marked an inline comment as done. MaskRay added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1610 static SectionType GetSectionTypeFromName(llvm::StringRef Name) { if (Name.consume_front(".debug_") ||

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. Why was this reverted? Please make extensive tests especially for something dealing with the build system. When reverting a commit, briefly describe what happened. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

MaskRay created this revision. MaskRay added reviewers: ckissane, labath. Herald added subscribers: StephenFan, emaste. Herald added a reviewer: alexander-shaposhnikov. Herald added a project: All. MaskRay requested review of this revision. Herald added a project: LLDB. Herald added a subscriber:

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: llvm/include/llvm/Support/Compression.h:54 + +void compress(StringRef InputBuffer, SmallVectorImpl , + int Level = DefaultCompression);

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

MaskRay added a comment. The patch looks mostly good but please fix the subject and the summary. Note: if you fix the local commit message, you can use ` arc diff --head=HEAD 'HEAD^' --verbatim` to update the subject and the summary. Comment at:

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

MaskRay added a comment. In D128465#3646561 , @ckissane wrote: > In D128465#3646258 , @MaskRay wrote: > >> In D128465#3646203 , @ckissane >> wrote: >> >>> In

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

MaskRay added a comment. In D128465#3646203 , @ckissane wrote: > In D128465#3642997 , @MaskRay wrote: > >> As I mentioned, the proper approach is to add zstd functionality along with >> the CMake change, instead

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added a comment. As I mentioned, the proper approach is to add zstd functionality along with the CMake change, instead of adding CMake to all llvm-project components without a way to test them. Comment at: lld/test/lit.site.cfg.py.in:21 config.have_zlib =

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay reopened this revision. MaskRay added a comment. This revision is now accepted and ready to land. This patch has changed a lot from what I have reviewed. The CMake change should be added along with `llvm::compression::zstd::*` functions. Otherwise the change just introduces some CMake

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added inline comments. Comment at: lld/test/lit.site.cfg.py.in:21 config.have_zlib = @LLVM_ENABLE_ZLIB@ +config.have_zstd = @LLVM_ENABLE_ZSTD@ config.have_libxar = @LLVM_HAVE_LIBXAR@ MaskRay wrote: > This needs a change in lld/test/CMakeLists.txt > >

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added inline comments. Comment at: lld/test/lit.site.cfg.py.in:21 config.have_zlib = @LLVM_ENABLE_ZLIB@ +config.have_zstd = @LLVM_ENABLE_ZSTD@ config.have_libxar = @LLVM_HAVE_LIBXAR@ This needs a change in lld/test/CMakeLists.txt Looks like you

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added a comment. Consider adding `have_zstd` to `compiler-rt/test/lit.common.cfg.py` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 ___ lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay accepted this revision. MaskRay added a comment. Please check both `LLVM_ENABLE_ZSTD={on,off}` work. Ideally, check that when zstd' cmake (`libzstd-dev` on Debian) is unavailable, `LLVM_ENABLE_ZSTD=on` works (there is no zstd functionality but the cmake invocation should succeed).

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

MaskRay added inline comments. Comment at: clang/lib/Basic/Targets/RISCV.h:144 + +StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E"; + gbenyei wrote: > MaskRay wrote: > > You may use a `char` and possibly fold this into the expression below. >

[Lldb-commits] [PATCH] D128465: Zstandard as a second compression method to LLVM

MaskRay added inline comments. Comment at: llvm/cmake/modules/FindZSTD.cmake:1 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# How did you derive this? The file seems contributed by you (I don't think facebook/zstd has such a file). It should not have

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

MaskRay added a comment. In D128612#3618167 , @gbenyei wrote: > In D128612#3617955 , @MaskRay wrote: > >> lld/ELF change should be dropped from this change. Don't use >> `config->endianness`. >> I feel sad that

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

MaskRay added a comment. lld/ELF change should be dropped from this change. Don't use `config->endianness`. I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-) Comment at:

[Lldb-commits] [PATCH] D128465: Zstandard as a second compression method to LLVM

MaskRay added a comment. In D128465#3612948 , @phosek wrote: > I think this patch should be broken into at least two: > > 1. Refactor `llvm/include/llvm/Support/Compression.h` and > `llvm/lib/Support/Compression.cpp` to introduce a generic interface and

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

MaskRay added a comment. I like the idea of `all` for llvm-objdump, but it should be a separate change, with `-mattr=-all` tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127741/new/ https://reviews.llvm.org/D127741

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

MaskRay added a comment. In D124673#3512190 , @phosek wrote: > In D124673#3512070 , @MaskRay wrote: > >> In D124673#3512037 , @paulkirth >> wrote: >> >>> Hi, Sorry for

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

MaskRay added a comment. In D124673#3512037 , @paulkirth wrote: > Hi, Sorry for the late notification, but I think this change may not apply > correctly to all configs. > > We're seeing a breakage in Fuchsia's Clang CI builders: >

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb1aed14bfea0: [llvm][lldb] use FindLibEdit.cmake everywhere (authored by upsj, committed by MaskRay). Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124673/new/ https://reviews.llvm.org/D124673

[Lldb-commits] [PATCH] D124760: [lldb] Fix ppc64 detection in lldb

MaskRay accepted this revision. MaskRay added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:315 + uint32_t endian = header.e_ident[EI_DATA]; + if(endian == ELFDATA2LSB) +return ArchSpec::eCore_ppc64le_generic;

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

MaskRay added a comment. I vaguely recall that some lldb bots use a stand-alone build. Say I have a build directory /tmp/RelA with `-DLLVM_ENABLE_PROJECTS=clang` cmake -GNinja -S~/llvm-project/lldb -Bout/lldb -DCMAKE_PREFIX_PATH=/tmp/RelA will print CMake Warning at

  1   2   3   >