[Lldb-commits] [PATCH] D137338: Fix dupe word typos

jhenderson added a comment. Herald added a reviewer: jdoerfert. Herald added subscribers: Michael137, sstefan1, JDevlieghere. This is going to be impossible to cleanly review as-is. Could it be broken into lots of smaller chunks? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jhenderson added a comment. FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual Studio generator, not ninja), and I don't have any issues - C++17 is being used. However, I currently only have LLD as an additional project enabled, and don't build compiler-rt.

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

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. In D130689#3684330 , @mehdi_amini wrote: > Nice, LGTM, thanks for driving this! > >> Remember that if we want to adopt some new feature in a

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

jhenderson added a comment. Objcopy aspects look good, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128612/new/ https://reviews.llvm.org/D128612 ___ lldb-commits mailing list

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

jhenderson added inline comments. Comment at: llvm/include/llvm/ADT/Triple.h:864 /// Tests whether the target is RISC-V (32- and 64-bit). bool isRISCV() const { Perhaps worth updating to mention big and little endian here, like `isPPC64` above?

[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes

jhenderson accepted this revision. jhenderson added a comment. LGTM, with suggested nits. Comment at: llvm/docs/DeveloperPolicy.rst:188 +notes, typically found in ``docs/ReleaseNotes.rst`` for the project. Changes to +a project that are user-facing or users may wish to know

[Lldb-commits] [PATCH] D116351: Update Bug report URL to Github Issues

jhenderson accepted this revision. jhenderson added a comment. LGTM, from my point of view. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116351/new/ https://reviews.llvm.org/D116351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D116351: Update Bug report URL to Github Issues

jhenderson added inline comments. Comment at: llvm/docs/CommandGuide/llvm-install-name-tool.rst:79 -To report bugs, please visit . +To report bugs, please visit . I believe

[Lldb-commits] [PATCH] D105134: [jenkins] Update script to use cross-project lit test suite

jhenderson accepted this revision. jhenderson added a comment. Looks good, with the exception of my inline comment. Comment at: zorg/jenkins/build.py:619 run_cmd(conf.lldbbuilddir(), ['/usr/bin/env', 'TERM=vt100', NINJA, '-v', -

[Lldb-commits] [PATCH] D102630: [lit] Stop using PATH to lookup clang/lld/lldb unless requested

This revision was automatically updated to reflect the committed changes. Closed by commit rGa1e656585578: [lit] Stop using PATH to lookup clang/lld/lldb unless requested (authored by jhenderson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D102630: [lit] Stop using PATH to lookup clang/lld/lldb unless requested

jhenderson created this revision. jhenderson added reviewers: JDevlieghere, aprantl, MaskRay, thopre, dblaikie, teemperor. Herald added subscribers: usaxena95, kadircet, delcypher. jhenderson requested review of this revision. Herald added subscribers: lldb-commits, ilya-biryukov, aheejin. Herald

[Lldb-commits] [PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, with nit. Comment at: llvm/lib/Support/MemoryBuffer.cpp:275 + return getFileAux( + Filename, /*MapSize=*/-1, 0, /*IsText=*/false, +

[Lldb-commits] [PATCH] D99182: [NFC] Reordering parameters in getFile and getFileOrSTDIN

jhenderson added a comment. Overall, this seems reasonable to me. I have a slight concern that it might cause surprising failures for downstream users, that aren't picked up at build time (due to implicit conversions), but I don't think you need to worry too much about that.

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. In D98179#2626179 , @davezarzycki wrote: > Might somebody be willing to sign off on this change (this week or next)? I'd > like to

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

jhenderson added inline comments. Comment at: llvm/utils/lit/lit/Test.py:217 +for line in time_file.readlines(): +time, path = line.split(' ', 1) +self.test_times[path.strip('\n')] = float(time) jhenderson

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

jhenderson added a comment. In D98179#2615130 , @yln wrote: > can we model "test failed" explicitly instead of making their execution time > really large? I am strongly in favour of this, if it can be done. My team have wanted an option to rerun just

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

jhenderson added a comment. I've skimmed the implementation, and it looks good to me, but I haven't got the time right now to review thoroughly. I'm pleased to see the ease of implementing "only-failures" on top of this too, thanks for illustrating. Comment at:

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

jhenderson added inline comments. Comment at: llvm/utils/lit/lit/Test.py:216 +with open(test_times_file, 'r') as time_file: +for line in time_file.readlines(): +time, path = line.split(' ', 1) I believe you don't

[Lldb-commits] [PATCH] D87878: [DWARFYAML] Make the include_directories, file_names and opcodes fields of the line table optional.

jhenderson accepted this revision. jhenderson 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/D87878/new/ https://reviews.llvm.org/D87878

[Lldb-commits] [PATCH] D87878: [DWARFYAML] Make the include_directories, file_names and opcodes fields of the line table optional.

jhenderson added a comment. I might be missing it, but do you have direct testing showing that the default for `IncludeDirs`\`Files`\`Opcodes` is an empty output, when the output is written? I think it's important that this is tested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D85289: [DWARFYAML][debug_info] Rename some mapping keys. NFC.

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Okay, LGTM. I don't mind either way, and I suspect with the offset field becoming optional soon, it's unlikely to appear frequently, so the verbosity is a non-issue then.

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

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

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:658 + +# NOTABLE: yaml2obj: error: abbrev table index must be less than or equal to the

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

jhenderson added a comment. I'm not sure if my test comments have been looked at. Could you mark them as Done if you think you have addressed them, please? Comment at: llvm/include/llvm/ObjectYAML/DWARFYAML.h:234-237 + Expected getAbbrevTableIndexByID(uint64_t ID); +

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for referencing different abbrev tables.

jhenderson added inline comments. Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34 +private: + Data + std::map AbbrevID2Index; Higuoxing wrote: > labath wrote: > > jhenderson wrote: > > > Would it make any sense to merge the `DWARFYAML::Data` class

[Lldb-commits] [PATCH] D86261: Add hashing of the .text section to ProcessMinidump.

jhenderson resigned from this revision. jhenderson added a comment. I'm not an LLDB developer, so probably not the right person to review this, sorry. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86261/new/ https://reviews.llvm.org/D86261

[Lldb-commits] [PATCH] D86194: [DWARFYAML] Add support for emitting multiple abbrev tables.

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, except for the new test change - I think that should be in a different patch. Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_abbrev.yaml:1 +## Test that

[Lldb-commits] [PATCH] D86194: [DWARFYAML] Add support for emitting multiple abbrev tables.

jhenderson added inline comments. Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:100 + for (const DWARFYAML::AbbrevTable : DI.DebugAbbrev) { +for (auto AbbrevDecl : AbbrevTable.Table) { + AbbrevCode = Don't use `auto` here. It's not obvious what the

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for emitting multiple abbrev tables.

jhenderson added inline comments. Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34 +private: + Data + std::map AbbrevID2Index; Would it make any sense to merge the `DWARFYAML::Data` class and `DWARFYAML::DWARFState` class at all?

[Lldb-commits] [PATCH] D85289: [DWARFYAML][debug_info] Rename some mapping keys. NFC.

jhenderson added a subscriber: labath. jhenderson added a comment. I see the point, but we don't do it for all fields in other contexts, and I have some mild concerns that `DebugAbbrevOffset` is unnecessarily verbose (I'd think `AbbrevOffset` would be sufficient. Perhaps it would be best to

[Lldb-commits] [PATCH] D85289: [DWARFYAML][debug_info] Rename some mapping keys. NFC.

jhenderson added a comment. What's the motivation for doing this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85289/new/ https://reviews.llvm.org/D85289 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D84008: [DWARFYAML] Refactor emitDebugInfo() to make the length be inferred.

jhenderson added inline comments. Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:380-382 + cantFail(writeVariableSizedInteger(Unit.AbbrOffset, + Unit.Format == dwarf::DWARF64 ? 8 : 4, + OS,

[Lldb-commits] [PATCH] D84008: [DWARFYAML] Refactor emitDebugInfo() to make the length be inferred.

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, thanks, but might be best to get a second opinion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84008/new/

[Lldb-commits] [PATCH] D84008: [DWARFYAML] Refactor emitDebugInfo() to make the length be inferred.

jhenderson added inline comments. Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:743 +# RUN: llvm-readelf --hex-dump=.debug_info %t13.o | \ +# RUN: FileCheck %s --check-prefix=LENGTH + Should this be INFER-LENGTH? (The test is currently

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

jhenderson added a comment. In principle, this approach looks fine, but please do as @labath suggests to reduce the amount of changes in one go. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84008/new/ https://reviews.llvm.org/D84008

[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for emitting multiple abbrev tables.

jhenderson added a comment. In D83116#2130019 , @labath wrote: > What would you say if, instead of `AbbrevTableIndex`, we had a field like > `AbbrevTableID`. The main difference would be that this "ID" field can be > explicitly specified on the Abbrev

[Lldb-commits] [PATCH] D82622: [DWARFYAML][debug_info] Replace 'InitialLength' with 'Format' and 'Length'.

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Forgot to mark as accepted before, but one more comment to add. Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:515 -#CHECK: DWARF:

[Lldb-commits] [PATCH] D82622: [DWARFYAML][debug_info] Replace 'InitialLength' with 'Format' and 'Length'.

jhenderson added a comment. LGTM. Well done on catching the lldb failures - I don't even build lldb by default personally. Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:515 -#CHECK: DWARF: -#CHECK: debug_info: -#CHECK: - Length:

[Lldb-commits] [PATCH] D77302: [DebugInfo] Rename getOffset() to getContribution(). NFC.

jhenderson accepted this revision. jhenderson 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/D77302/new/ https://reviews.llvm.org/D77302

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

jhenderson accepted this revision. jhenderson added a comment. Nothing else from me, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75929/new/ https://reviews.llvm.org/D75929 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

jhenderson added a comment. In D74023#1949261 , @HsiangKai wrote: > In D74023#1948388 , @MaskRay wrote: > > > The code generally looks good. For unittests, I think we can either make > > llvm-readobj -A canonical

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

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. I haven't really reviewed the funcional parts of this change in the attribute parser stuff, but everything else LGTM. Please wait for somebody else to review the attribute parser

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

jhenderson added a comment. In D74023#1937220 , @HsiangKai wrote: > In D74023#1933427 , @jhenderson > wrote: > > > @HsiangKai, have you noticed that there are some test failures according to > > the harbourmaster

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

jhenderson added a comment. @HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow. Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:21 + Error handler(uint64_t tag,

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

jhenderson added a comment. Thanks. My comments have all been addressed. I'm happy once somebody else looks at the more technical aspects. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75929/new/ https://reviews.llvm.org/D75929 ___

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

jhenderson added inline comments. Comment at: llvm/test/DebugInfo/X86/dwp-v5-cu-index.s:1 +## The test checks that we can parse and dump a CU index section that compliant +## to the DWARFv5 standard. that is compliant Comment at:

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

jhenderson added inline comments. Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1727 + + if (Tag % 2) +IsIntegerValue = false; I don't understand why this line changed, but more importantly, the `2` looks like a magic number and it is

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

jhenderson added a comment. I'm not really up to speed with the .debug_*_index sections, so I haven't looked really at the overall approach. I've just provided some basic stylistic comments. Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:22 +// Pre-standard

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

jhenderson added inline comments. Comment at: llvm/include/llvm/Support/ELFAttributes.h:32 + bool HasTagPrefix = true); +Optional attrTypeFromString(StringRef Tag, TagNameMap Map); + HsiangKai wrote: > jhenderson wrote: > > Can this be

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

jhenderson added inline comments. Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:5 +#include "gtest/gtest.h" +#include + jhenderson wrote: > I think it's normal to put a blank line between standard library headers and > other includes. Ignore this.

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

jhenderson marked an inline comment as done. jhenderson added inline comments. Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:323-325 +// Treat this error as unrecoverable - we cannot be sure what any of +// the data represents including the length field, so

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

This revision was automatically updated to reflect the committed changes. Closed by commit rG7116e431c0ab: [DebugInfo] Make most debug line prologue errors non-fatal to parsing (authored by jhenderson). Changed prior to commit: https://reviews.llvm.org/D72158?vs=240852=241076#toc Repository:

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

jhenderson added a comment. Thanks for the review comments! I'll go ahead and land it like this, assuming my local test run passes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72158/new/ https://reviews.llvm.org/D72158

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

jhenderson requested review of this revision. jhenderson added a comment. In D72158#1844534 , @labath wrote: > If I understand this correctly, this will cause lldb to continue to read the > parsed line table contribution after encountering some errors in

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

jhenderson reopened this revision. jhenderson added a comment. This revision is now accepted and ready to land. Could somebody please look at the LLDB change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72158/new/ https://reviews.llvm.org/D72158

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

jhenderson updated this revision to Diff 240852. jhenderson added a reviewer: labath. jhenderson added a comment. Herald added subscribers: lldb-commits, emaste. Herald added a reviewer: espindola. Herald added a project: LLDB. Fix LLD test + fix LLDB build. I'm uncertain if the LLDB fix is the

[Lldb-commits] [PATCH] D68645: MinidumpYAML: Add support for the memory info list stream

jhenderson added subscribers: MaskRay, grimar. jhenderson added a comment. FYI, I'm going to be away for 2 and a half weeks from the end of work today, so won't have time to look at these if I don't get to them later today. I have no issues with other people reviewing them. You might want to

[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Thanks, LGTM (modulo the Minidump-specific details). Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68210/new/ https://reviews.llvm.org/D68210

[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream

jhenderson added inline comments. Comment at: lib/Object/Minidump.cpp:58 +MinidumpFile::getMemoryInfoList() const { + auto OptionalStream = getRawStream(StreamType::MemoryInfoList); + if (!OptionalStream) labath wrote: > jhenderson wrote: > > I probably should

[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream

jhenderson added a comment. Can't comment too much on the file format details, but I've made some more general comments. FYI, I'll be away from end of day Wednesday for 2 and a half weeks, so won't be able to further review after that point until I'm back. Comment at:

[Lldb-commits] [PATCH] D61885: Minidump: Add support for the MemoryList stream

jhenderson added inline comments. Comment at: include/llvm/Object/Minidump.h:85-86 + /// error is returned if the file does not contain this stream, or if the + /// stream is not large enough to contain the number of threads declared in + /// the stream header. The

[Lldb-commits] [PATCH] D61423: MinidumpYAML: add support for the ThreadList stream

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM, with the suggested fixes. Comment at: test/tools/obj2yaml/basic-minidump.yaml:47-49 + - Thread Id: 0x5C5D5E5F +Priority Class: 0x60616263 +

[Lldb-commits] [PATCH] D61423: MinidumpYAML: add support for the ThreadList stream

jhenderson added inline comments. Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:57 +/// instantiations can be used to represent the ModuleList stream and other +/// streams with similar structure. +template with -> with a Comment at:

[Lldb-commits] [PATCH] D61064: Object/Minidump: Add support for the ThreadList stream

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Aside from a couple of nits, LGTM. Comment at: lib/Object/Minidump.cpp:69 size_t ListOffset = 4; - // Some producers insert additional padding bytes to align the

[Lldb-commits] [PATCH] D60405: MinidumpYAML: Add support for ModuleList stream

jhenderson accepted this revision. jhenderson added inline comments. Comment at: lib/ObjectYAML/MinidumpYAML.cpp:21 +/// methods, while the final minidump file is written by calling the writeTo +/// method. The plan versions of allocation functions take a reference to the +///

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59775/new/ https://reviews.llvm.org/D59775 ___

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

jhenderson added a comment. Code basically looks fine, but somebody else should confirm that the format is correct. Comment at: lib/Object/Minidump.cpp:55 + if (!OptionalStream) +return createError("No such stream", object_error::invalid_section_index); + auto

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

jhenderson added inline comments. Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70 minidump::SystemInfo Info; + std::string CSDVersion; Don't know about lifetime here, but could this be a `StringRef`? Comment at:

[Lldb-commits] [PATCH] D59634: Add minidump support to obj2yaml

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. In D59634#1449621 , @labath wrote: > James, do you have any thoughts on this? I think the minidump-specific > changes are all pretty

[Lldb-commits] [PATCH] D59482: [ObjectYAML] Add basic minidump generation support

jhenderson accepted this revision. jhenderson added a comment. Test updates LGTM. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59482/new/ https://reviews.llvm.org/D59482 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D59482: [ObjectYAML] Add basic minidump generation support

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. LGTM. I don't really know much about the minidump stuff, but the general structure looks fine to me. Repository: rL LLVM CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D59482: [ObjectYAML] Add basic minidump generation support

jhenderson added a comment. I've not reviewed the unit test yet, but the bulk of the body looks fine, apart from some minor details. Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:27 +struct Stream { + enum StreamKind { +HexKind, I think it is worth

[Lldb-commits] [PATCH] D59291: [Object] Add basic minidump support

jhenderson added a comment. I haven't really looked at the behaviour to make sure it makes sense, but I've made a number of comments on the comments and one or two other things. Comment at: include/llvm/Object/Minidump.h:25 + /// Construct a new MinidumpFile object from the

[Lldb-commits] [PATCH] D59291: Add basic minidump support to obj2yaml

jhenderson added a comment. In D59291#1427349 , @labath wrote: > So, if we want to split this up more, I can propose the following: > > - split off the BinaryFormat changes into a separate patch, test it > with some inline C char arrays > - write the

[Lldb-commits] [PATCH] D59291: Add basic minidump support to obj2yaml

jhenderson added a comment. Is it possible to unit-test some of the Object and BinaryFormat changes? That would allow you to split those parts away from the obj2yaml changes. Also, if you added support in yaml2obj, you could test that, then use that in testing the obj2yaml changes more easily,