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

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

[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] 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] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. `ninja clang` on Windows works. `check-llvm-tools` has a few `REQUIRES: system-windows` tests and I am running them. Test invocation is bit slow. Repository: rG LLVM Github Monorepo CHAN

[Lldb-commits] [PATCH] D108233: WIP: Add minidump save-core functionality to ELF object files

MaskRay added inline comments. Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:120 + if (error.Fail()) { +error.SetErrorString("Unable to convert the csd string to UTF16."); +return error; https://llvm.org/docs/CodingStandards

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

MaskRay added a subscriber: felixonmars. MaskRay added a comment. Hi Luis, is this still needed after D86292 ? Or are there missing pieces? @felixonmars reported that https://archriscv.felixc.at/.status/logs/lldb.log still failed to build on riscv64 Arch Linux. I

[Lldb-commits] [PATCH] D109779: [LLDB] [Minidump] Fix format string warnings on Windows

MaskRay added a comment. I fixed this in e69d359841b6358f1d17569212ef8cf91244ca11 and fixed some style issues. --- This needs extra care. While Clang -Wformat flags printf("%llu", (size_t)3); warning: format specifi

[Lldb-commits] [PATCH] D109975: [CMake] Consistently use the LibXml2::LibXml2 target instead of LIBXML2_LIBRARIES

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

[Lldb-commits] [PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

MaskRay accepted this revision. MaskRay added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111454/new/ https://reviews.llvm.org/D111454 ___ lldb-commits mailing list lldb-commits@list

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

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

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

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

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

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

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

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

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

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

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

[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 https://reviews.ll

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

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

[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] 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 "llvm/

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

MaskRay added a comment. Can `createMCObjectFileInfo` return `MCObjectFileInfo` instead of `std::unique_ptr`? Comment at: clang/lib/Parse/ParseStmtAsm.cpp:590 + if (!MAI || !MII || !MOFI || !STI) { Diag(AsmLoc, diag::err_msasm_unable_to_create_target)

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. This looks like an improvement because it avoids a temporary `MCObjectFileInfo MOFI;` (which appeared to be initialized in two subsequent calls) in numerous places. Repository: rG

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/TargetRegistry.h:26 #include "llvm/ADT/iterator_range.h" +#include "llvm/MC/MCObjectFileInfo.h" #include "llvm/Support/CodeGen.h" `include/llvm/Support/TargetRegistry.h now has cyclic dependen

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

MaskRay added subscribers: compnerd, dblaikie. MaskRay added a comment. Because of `new MCObjectFileInfo`, we cannot use a forward declaration (incomplete class) to replace `#include "llvm/MC/MCObjectFileInfo.h"` in `TargetRegistry.h`. I thought about moving `TargetRegistry.{h,cpp}` from Suppor

[Lldb-commits] [PATCH] D101921: [MC] Make it possible for targets to define their own MCObjectFileInfo

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc2f819af73c5: [MC] Refactor MCObjectFileInfo initialization and allow targets to create… (authored by flip1995, committed by MaskRay). Repository:

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

MaskRay added inline comments. Comment at: llvm/include/llvm/ADT/StringRef.h:192 -/// equals_lower - Check for string equality, ignoring case. +/// equals_insensitive - Check for string equality, ignoring case. LLVM_NODISCARD https://llvm.org/docs/

[Lldb-commits] [PATCH] D106339: Add support to generate Sphinx DOCX documentation

MaskRay added a comment. I remember that we even converted some `*.md` to `*.rst` to remove the number of supported documentation formats, so I agree that adding support the even less used `*.docx` may not be the proper direction. Thanks for splitting this off. Repository: rG LLVM Github Mo

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added a comment. Drive-by: std::filesystem is unavailable for GCC<8. The minimum GCC version is 5, so std::filesystem isn't suitable. You may use`include/llvm/Support/FileSystem.h` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added inline comments. Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 +std::string proc_fd_path = "/proc/self/fd"; +std::filesystem::path fp(proc_fd_path); /proc/self/fd is generally unavailable on non-Linux OSes. CHANGES

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay accepted this revision. MaskRay added a comment. LGTM. Some nits Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:161 +// stdin/stdout/stderr +if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd) + files_to_close.push_b

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added inline comments. Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 + +std::string proc_fd_path = "/proc/self/fd"; +std::error_code EC; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

MaskRay added inline comments. Comment at: lldb/test/API/functionalities/dyld-launch-linux/Makefile:1 +CXX_SOURCES := main.cpp +LD_EXTRAS := -Wl,-rpath "-Wl,$(shell pwd)" ``` CXX_SOURCES := main.cpp DYLIB_NAME := signal_file DYLIB_CXX_SOURCES := signal_file.cpp

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

MaskRay added inline comments. Comment at: lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py:16 +exe = "/lib64/ld-linux-x86-64.so.2" +if(os.path.exists(exe)): +target = self.dbg.CreateTarget(exe) ==

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

MaskRay added inline comments. Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:293 + // ld.so saves empty file name for the executable file in the link map. + // When executable is run using ld.so, we need to be update executable path. + if (name.em

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to readability-identifier-naming.{Function, Member, Parameter, Variable}Case

MaskRay created this revision. MaskRay added reviewers: JDevlieghere, jingham, labath. Herald added subscribers: lldb-commits, aheejin. Herald added a project: LLDB. MaskRay added a comment. https://github.com/google/llvm-premerge-checks/issues/142 This should suppress the annoying `clang-tidy li

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to readability-identifier-naming.{Function, Member, Parameter, Variable}Case

MaskRay added a comment. https://github.com/google/llvm-premerge-checks/issues/142 This should suppress the annoying `clang-tidy linux` diagnostics in `Diff Detail - Build Status`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75810/new/ https://r

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to disable readability-identifier-naming

MaskRay updated this revision to Diff 249139. MaskRay retitled this revision from "[lldb] Add .clang-tidy with customization to readability-identifier-naming.{Function,Member,Parameter,Variable}Case" to "[lldb] Add .clang-tidy with customization to disable readability-identifier-naming". MaskRay

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to disable readability-identifier-naming

This revision was automatically updated to reflect the committed changes. Closed by commit rG71269a1f172c: [lldb] Add .clang-tidy with customization to disable readability-identifier… (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

MaskRay added a comment. commit fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 should be associated with `Differential Revision: https://reviews.llvm.org/D76111`. The revert of fd868f517d2c5ca8c0f160dbec0857b14ecf74c1

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

MaskRay added a comment. I remember the patch caused many failures yesterday. The new diff is good. Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:16 + +TagNameMap emptyTagNameMap; + `static const` Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

MaskRay added inline comments. Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:102 + +Streamer.emitIntValue(ELFAttrs::Format_Version, 1); + } `emitInt8` Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cp

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

MaskRay added a comment. The code generally looks good. For unittests, I think we can either make llvm-readobj -A canonical or the unittests canonical. If we decide to place tests on one place, we should delete most tests on the other side. My current preference is that we use more of unittests

[Lldb-commits] [PATCH] D77186: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one

MaskRay added a comment. In D77186#1955451 , @wallace wrote: > address comments @wallace labath made comments after the differential had been approved. So it looks like he may have further questions. It was probably better waiting for an explicit ack i

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

MaskRay added a comment. I am supportive of getting rid of InlineAsmDiagnosticHandler, too. The updated AMDGPU tests suggeste that previously `MCContext::reportError` may be called with no `SrcMgr` or `InlineSrcMgr`, so the error is propagated to the temporary `SourceMgr()`. The `LLCDiagnosticH

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:477 + StringRef Message = D.getMessage(); + if (Message.startswith("error: ")) +Message = Message.substr(7); `StringRef::consume_front` I know you are moving code, but do you kn

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

MaskRay added a comment. Is the https://github.com/ClangBuiltLinux/linux/issues/1269 unreachable error fixed? Reproduce: git clone https://android.googlesource.com/kernel/common.git --branch android-4.19-stable cd common PATH=path/to/your/clang/bin:$PATH make -s -j 30 LLVM=1 O=/tmp/out/a

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

MaskRay added a comment. LG. Can you attach an example triggering clang `InlineAsmDiagHandler2`? I want to check what the diagnostic looks like now. Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:22 def note_fe_inline_asm_here : Note<"instantiated into assem

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

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/D97449/new/ https://reviews.llvm.org/D97449 __

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/include/llvm/MC/MCContext.h:67 class SourceMgr; + template class function_ref; With `std::function`, this can be dropped. Repository: rG LLVM Github Monorepo CHANGES SINCE

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

MaskRay added a comment. This touches a lot of files. I am a bit worried that it would not be easy for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy. > On SystemZ we need to open text files in text mode, Why can't it be served without CRLF translation? Repository: rG

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

MaskRay added a comment. In D99426#2653869 , @abhina.sreeskantharajan wrote: > In D99426#2653725 , @MaskRay wrote: > >> This touches a lot of files. I am a bit worried that it would not be easy >> for a contributo

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

MaskRay added a comment. In D99426#2656217 , @rnk wrote: > In D99426#2653725 , @MaskRay wrote: > >> This touches a lot of files. I am a bit worried that it would not be easy >> for a contributor to know OF_TextWith

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

MaskRay added a comment. /// The file should be opened in text mode on platforms like z/OS that make /// this distinction. OF_Text = 1, F_Text = 1, // For compatibility /// The file should use a carriage linefeed '\r\n'. /// Only makes a difference on windows. OF_CRLF = 2, //

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. In D99426#2664883 , @abhina.sreeskantharajan wrote: > In D99426#2664841 , @MaskRay wrote: > >> /// The fil

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

MaskRay created this revision. MaskRay added reviewers: labath, rupprecht. Herald added a reviewer: shafik. MaskRay requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. This fixes 6 check-lldb-shell failures in a `-DLLVM_USE_SANITIZER=Leaks` bui

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

MaskRay updated this revision to Diff 338664. MaskRay marked an inline comment as done. MaskRay added a comment. buf -> name_buf Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100800/new/ https://reviews.llvm.org/D100800 Files: lldb/source/Plugin

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

MaskRay added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1229 if (!function_decl) { +char *buf = nullptr; llvm::StringRef name = attrs.name.GetStringRef(); teemperor wrote: > `name_buf` ? From

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

MaskRay added a comment. In D100800#2699940 , @teemperor wrote: > Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot > for LLDB... > > I just have some minor comments but otherwise this LGTM. Agree.. The 45+ `check-lldb` failur

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa2cd6d07691a: [lldb] Fix demangler leaks in the DWARF AST parser (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

MaskRay added a comment. The remaining failures are on Linux x86-64 in a `-DLLVM_USE_SANITIZER=Leaks` build (`Address` should achieve the same effect because on many targets asan enables LeakSanitizer) Failed Tests (45): lldb-shell :: Reproducer/Functionalities/TestDataFormatter.test

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

MaskRay created this revision. MaskRay added reviewers: JDevlieghere, rupprecht, shafik, teemperor. MaskRay requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Use a variable of static storage duration to reference an intentionally leaked varia

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

MaskRay added a comment. @JDevlieghere The Reproducer library seems to have several other leaks. Maybe you'd like to look at them? :) I am unfamiliar with the code... ==2013285==ERROR: LeakSanitizer: detected memory leaks Direct leak of 32 byte(s) in 2 object(s) allocated from: #0 0

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

MaskRay updated this revision to Diff 338688. MaskRay marked an inline comment as done. MaskRay added a comment. remove a stale comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100806/new/ https://reviews.llvm.org/D100806 Files: lldb/tools/

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcdae6d7711d6: [lldb] Fix one leak in reproducer (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

MaskRay marked an inline comment as done. MaskRay added inline comments. Comment at: lldb/tools/driver/Driver.cpp:856 if (const char *reproducer_path = SBReproducer::GetPath()) { -// Leaking the string on purpose. -std::string *finalize_cmd = new std::strin

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

MaskRay marked 2 inline comments as done. MaskRay added inline comments. Comment at: lldb/tools/driver/Driver.cpp:856 if (const char *reproducer_path = SBReproducer::GetPath()) { -// Leaking the string on purpose. -std::string *finalize_cmd = new std::strin

[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo

MaskRay added a comment. The refactoring part seems useful on its own. The controversial 4-byte alignment part should be split. Doesn't GNU as use 4 for most architectures as well? Why do you want to change it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo

MaskRay added a comment. The `InitMCObjectFileInfo` refactoring is good, but I'll suggest `initMCObjectFileInfo(MCContext &ctx, bool PIC, bool LargeCodeModel)` (change the function name to `lowerCase`, and reorder MCContext before bool arguments). The refactoring adding `Triple` to MCContext::M

[Lldb-commits] [PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG632ebc4ab437: [MC] Untangle MCContext and MCObjectFileInfo (authored by flip1995, committed by MaskRay). Repository: rG LLVM Github Monorepo CHAN

[Lldb-commits] [PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

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

[Lldb-commits] [PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

MaskRay added a comment. In D101462#2733726 , @flip1995 wrote: >> I'll keep this open for a few days as it touches too many things. > > Sounds good 👍 > >> but you'll need to provide your name and email > > ~~I used `arc diff`. The commits I made with `git

[Lldb-commits] [PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

MaskRay added a comment. In D101462#2733691 , @flip1995 wrote: > Not sure how the process from here on out is. I think it is important to > note, that I don't have push rights and someone else will have to land this > for me (I guess?). I'll keep this

[Lldb-commits] [PATCH] D68541: Implement 'up' and 'down' shortcuts in lldb gui

MaskRay added a comment. What is the state of the patch? Does lldb support cgdb-style u/d/f/b etc now? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68541/new/ https://reviews.llvm.org/D68541 ___ lldb-commits maili

[Lldb-commits] [PATCH] D84402: [lldb/DWARF] Add more line table validation

MaskRay added a comment. For ELF, there are non-pic cases (i.e. -no-pie) and pic cases (-pie or -shared). I think it is sufficient just testing -pie (image base is zero). If filtering for -pie works, filter for -no-pie or -shared should work as well. Comment at: lldb/source/P

[Lldb-commits] [PATCH] D84401: [lldb] Add SectionList::ContainsFileAddressRange

MaskRay added inline comments. Comment at: lldb/source/Core/Section.cpp:570 + while (file_addr < end) { +SectionSP sect_sp = FindSectionContainingFileAddress(file_addr); +if (!sect_sp) I am a bit concerned about the potential large time complexity here.

[Lldb-commits] [PATCH] D85169: [test] Exit with an error if no tests are run.

MaskRay added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:1043 +if configuration.suite.countTestCases() == 0: +print("error: did not discover any tests.") +exitTestSuite(1) JDevlieghere wrote: > Maybe `did not di

[Lldb-commits] [PATCH] D85248: [test] Support git commit version ids for clang.

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/D85248/new/ https://reviews.llvm.org/D85248

[Lldb-commits] [PATCH] D85100: [ELF] Allow sections after a non-SHF_ALLOC section to be covered by PT_LOAD

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rGa6db64ef4a99: [ELF] Allow sections after a non-SHF_ALLOC section to be covered by PT_LOAD (authored by MaskRay). Herald

[Lldb-commits] [PATCH] D85968: [lldb] Forcefully complete a type when adding nested classes

MaskRay added inline comments. Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s:7 -# RUN: llvm-mc --triple x86_64-pc-linux %s --filetype=obj > %t -# RUN: %lldb %t -o "expr a" -o exit 2>&1 | FileCheck %s --check-prefix=EXPR -# RUN: %lldb %t -o "targ

[Lldb-commits] [PATCH] D86857: [Test] Simplify DWARF test cases. 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/D86857/new/ https://reviews.llvm.org/D86857

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

MaskRay added inline comments. Comment at: llvm/lib/Support/MemoryBuffer.cpp:460 - if (shouldUseMmap(FD, FileSize, MapSize, Offset, RequiresNullTerminator, -PageSize, IsVolatile)) { + if (false) { std::error_code EC; I guess this was

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

MaskRay added inline comments. Comment at: lldb/source/Target/TraceSettingsParser.cpp:123 + if (schema.empty()) { +std::ostringstream schema_builder; +schema_builder << "{\n \"trace\": "; This requires `sstream`. Fixed. Comment at: lld

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

MaskRay added inline comments. Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149 + int r = ptsname_r(m_primary_fd, buf, sizeof(buf)); + assert(r == 0); + return buf; labath wrote: > mgorny wrote: > > labath wrote: > > > mgorny wrote: > > > > I would r

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

MaskRay added inline comments. Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149 + int r = ptsname_r(m_primary_fd, buf, sizeof(buf)); + assert(r == 0); + return buf; MaskRay wrote: > labath wrote: > > mgorny wrote: > > > labath wrote: > > > > mgorny w

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Worth mentioning that this will be in POSIX issue 8 https://www.austingroupbugs.net/bug_view_page.php?bug_id=508 Comment at: lldb/source/Host/common/PseudoTerminal.c

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

MaskRay added inline comments. Comment at: lldb/tools/lldb-server/CMakeLists.txt:61 LINK_COMPONENTS Support ) Otherwise it fails in a BUILD_SHARED_LIBS=on build because the -Wl,-z,defs linker option requires a module to have fully specified dependen

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

MaskRay added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:404 + unsigned MissingArgCount; + opt::InputArgList Args = Opts.ParseArgs(makeArrayRef(argv + 2, argc - 2), + MissingArgIndex, MissingArgCount); ---

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

MaskRay added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:369 + + If no target is selected a startup, the lldb-server can be directed to launch + or attach a process by the LLDB client. `a startup` -> `at startup` Repository: rG L

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

MaskRay added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:405 + bool HasError = false; + opt::InputArgList Args = Opts.parseArgs(argc - 1, argv + 1, OPT_UNKNOWN, + Saver, [&](llvm::StringRef Msg) {

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

MaskRay added a comment. This was probably due to: Expected(Error Err) : HasError(true) #if LLVM_ENABLE_ABI_BREAKING_CHECKS // Expected is unchecked upon construction in Debug builds. , Unchecked(true) #endif { assert(Err && "Cannot create Expected fr

<    1   2   3   >