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

[James Henderson via Phabricator via lldb-commits]

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, rather than relying on a canned binary.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59291/new/

https://reviews.llvm.org/D59291



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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&Object changes into a separate patch, test it 
> with some inline C char arrays
> - write the yaml2obj equivalent of this patch, test it via unit tests and the 
> Object minidump reader
> - finally, come back to this patch (obj2yaml), test it via `yamlobj|obj2yaml` 
> round-tripping
>
>   What do you think?


This sounds like a reasonable breakdown to me. Another possibility for testing 
yaml2obj is some form of minidump support in llvm-readobj or similar, which 
uses the Object minidump reader to verify things in lit tests.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59291/new/

https://reviews.llvm.org/D59291



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 given memory buffer. Returns
+  /// an error if this file cannot be identified as a minidump file, or if it's
+  /// contents are badly corrupted (i.e., we cannot read the stream directory).

it's -> its



Comment at: include/llvm/Object/Minidump.h:26
+  /// an error if this file cannot be identified as a minidump file, or if it's
+  /// contents are badly corrupted (i.e., we cannot read the stream directory).
+  static Expected> create(MemoryBufferRef 
Source);

Nit, you don't need the ',' after 'i.e.'.



Comment at: include/llvm/Object/Minidump.h:42
+
+  /// Returns raw the contents of the stream of the given type, or None if the
+  /// file does not contain a stream of this type.

raw the -> the raw



Comment at: include/llvm/Object/Minidump.h:61
+  static Error createEOFError() {
+return createError("Unexpected EOF.", object_error::unexpected_eof);
+  }

I've seen in some places that the error handling automatically appends a full 
stop, so this would result in two full stops, which is probably not ideal. 
Also, having a full stop prevents a consumer producing the error message in the 
middle of a quote or similar, e.g. "the library produced the following error 
'Unexpected EOF' and will terminate" or whatever.

Same comment applies in a few other places.



Comment at: include/llvm/Object/Minidump.h:68
+
+  /// Return the slice of the given data array as an array of objects of given
+  /// type. The function checks that the input array is large enough to contain

of given -> of a given



Comment at: include/llvm/Object/Minidump.h:77
+   ArrayRef Streams,
+   std::unordered_map StreamMap)
+  : Binary(ID_Minidump, Source), Header(Header), Streams(Streams),

Are you deliberately making a copy of StreamMap? I would normally expect this 
to be passed by some form of reference.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59291/new/

https://reviews.llvm.org/D59291



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 making this an `enum class`.



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:55
+Callback(OS);
+  assert(OS.tell() == BeginOffset + NextOffset);
+  (void)BeginOffset;

Perhaps worth a quick message:

`assert(OS.tell() == BeginOffset + NextOffset && "some message here");`



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:70
+template 
+static inline void mapAs(yaml::IO &IO, const char *Key, EndianType &Val) {
+  MapType Mapped = static_cast(Val);

Maybe worth calling this `mapRequiredAs`, to line up with the `mapRequired` 
name.



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:96
+template 
+static inline void mapHex(yaml::IO &IO, const char *Key, EndianType &Val) {
+  mapAs::type>(IO, Key, Val);

Similar to above, perhaps `mapRequiredHex`?



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:100
+
+/// Perform an optionam yaml-mapping of an endian-aware type as an
+/// appropriately-sized hex value.

`optionam`?



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:181
+  FeaturesRef.writeAsBinary(FeaturesStream);
+  std::fill(Features.begin(), Features.end(), 0);
+  std::memcpy(Features.begin(), FeaturesStorage.begin(),

Is this definitely necessary? What's the type of Info.ProcessorFeatures? 
Similar comments for Vendor ID below.



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:183
+  std::memcpy(Features.begin(), FeaturesStorage.begin(),
+  std::min(FeaturesStorage.size(), Features.size()));
+}

If there is too much data to fit, should this emit an error, rather than 
silently truncate? Same below with Vendor ID.



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:306
+Error MinidumpYAML::writeAsBinary(StringRef Yaml, raw_ostream &OS) {
+  yaml::Input yin(Yaml);
+  Object Obj;

I don't think `yin` matches our (current) variable naming guidelines. It should 
be `Yin` at least, though it wasn't immediately clear what was meant even then 
at first to me, so perhaps a different name might be better (simply `Input` 
would work for me).



Comment at: tools/yaml2obj/yaml2obj.cpp:61
+  if (Doc.Minidump) {
+MinidumpYAML::writeAsBinary(*Doc.Minidump, Out);
+return 0;

I feel like it would make more sense for this to match the other versions 
above, i.e. I think this should be at least `return yaml2minidump(...);`


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59482/new/

https://reviews.llvm.org/D59482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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

https://reviews.llvm.org/D59482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 straight-forward here, so the thing I'm most 
> interested in is whether this is ok from a obj2yaml perspective.


Sorry, I've been a bit snowed under with other stuff recently. I don't claim to 
be an expert on obj2yaml, but this LGTM.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59634/new/

https://reviews.llvm.org/D59634



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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: unittests/Object/MinidumpTest.cpp:270
+  0,// Mis-align next string
+  2, 0, 0, 0, 'a', 0,   // String6 - "a"
+

What about a case where the size field cannot fit in the remaining data?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 ExpectedSize =

I wonder whether it would be worth a new class of errors for minidump files? 
After all, invalid_section_index feels a bit forced for a format without 
sections!



Comment at: unittests/Object/MinidumpTest.cpp:317
+  1, 0, 0, 0,   // NumberOfStreams,
+  0x20, 0, 0, 0,// StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp

It's a bit jarring seeing both hex and decimal numbers in here at the same 
time. Is there a particular reason you've used hex here, but decimal for e.g. 
116 below?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Okay, no more comments from me.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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
+/// data which is to be written (and hence the data must be available until

plan -> plain (?)



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:97
 
-  SmallVector EndianStr(WStr.size() + 1,
-  support::ulittle16_t());
-  copy(WStr, EndianStr.begin());
-  return allocateCallback(
-  sizeof(uint32_t) + EndianStr.size() * sizeof(support::ulittle16_t),
-  [EndianStr](raw_ostream &OS) {
-// Length does not include the null-terminator.
-support::ulittle32_t Length(2 * (EndianStr.size() - 1));
-OS.write(reinterpret_cast(&Length), sizeof(Length));
-OS.write(reinterpret_cast(EndianStr.begin()),
- sizeof(support::ulittle16_t) * EndianStr.size());
-  });
+  // The the utf16 string is null-terminated, but the terminator is not counted
+  // in the string size.

Double "the"


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60405/new/

https://reviews.llvm.org/D60405



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 llvm-install-name-tool is a variation of llvm-objcopy, so should 
probably reference the llvm-objcopy URL.



Comment at: llvm/docs/CommandGuide/llvm-otool.rst:135
 
-To report bugs, please visit .
+To report bugs, please visit .
 

I believe llvm-otool is a variation of llvm-objdump, so should reference the 
llvm-objdump URL.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116351/new/

https://reviews.llvm.org/D116351

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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?



Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:304-305
 {"elf64-littleriscv", {ELF::EM_RISCV, true, true}},
+{"elf32-bigriscv", {ELF::EM_RISCV, false, false}},
+{"elf64-bigriscv", {ELF::EM_RISCV, true, false}},
 // PowerPC

We need llvm-objcopy testing for these new targets.


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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 bigger way it should 
>> be discussed and added to the CodingStandard document.
>
> What does it mean exactly? We can't use **anything** C++17 without writing it 
> in the coding standards?
> I'm not sure it'll be manageable: how do you see this playing out?

Based on the release note, I don't think that was what was intended, although I 
am curious to understand what //was// intended!

Looks good to me anyway (but get more buy-in, perhaps a new Discourse post too, 
not just an update on the existing thread, since new threads get emailed out to 
more people).




Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:20
 
 # Map the above GCC versions to dates: 
https://gcc.gnu.org/develop.html#timeline
+set(LIBSTDCXX_MIN 7)

This comment seems out-of-date now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 about should be added
+to the project's release notes. Examples of changes that would typically





Comment at: llvm/docs/DeveloperPolicy.rst:192
+
+* Adding, removing, or modifying command line options.
+* Adding or removing a diagnostic.

I think official coding documentation style guides say to use "command-line" - 
at least, that's what our internal docs team has told me in the past.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123957/new/

https://reviews.llvm.org/D123957

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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

https://reviews.llvm.org/D137338

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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

https://reviews.llvm.org/D102630

Files:
  lldb/test/Shell/helper/toolchain.py
  llvm/utils/lit/lit/llvm/config.py

Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -401,10 +401,11 @@
 
 self.add_err_msg_substitutions()
 
-def use_llvm_tool(self, name, search_env=None, required=False, quiet=False):
+def use_llvm_tool(self, name, search_env=None, required=False, quiet=False,
+  use_installed=False):
 """Find the executable program 'name', optionally using the specified
-environment variable as an override before searching the
-configuration's PATH."""
+environment variable as an override before searching the build directory
+and then optionally the configuration's PATH."""
 # If the override is specified in the environment, use it without
 # validation.
 tool = None
@@ -412,7 +413,11 @@
 tool = self.config.environment.get(search_env)
 
 if not tool:
-# Otherwise look in the path.
+# Use the build directory version.
+tool = lit.util.which(name, self.config.llvm_tools_dir)
+
+if not tool and use_installed:
+# Otherwise look in the path, if enabled.
 tool = lit.util.which(name, self.config.environment['PATH'])
 
 if required and not tool:
@@ -429,11 +434,11 @@
 return tool
 
 def use_clang(self, additional_tool_dirs=[], additional_flags=[],
-  required=True):
+  required=True, use_installed=False):
 """Configure the test suite to be able to invoke clang.
 
 Sets up some environment variables important to clang, locates a
-just-built or installed clang, and add a set of standard
+just-built or optionally an installed clang, and add a set of standard
 substitutions useful to any test suite that makes use of clang.
 
 """
@@ -497,7 +502,8 @@
 
 # Discover the 'clang' and 'clangcc' to use.
 self.config.clang = self.use_llvm_tool(
-'clang', search_env='CLANG', required=required)
+'clang', search_env='CLANG', required=required,
+use_installed=use_installed)
 if self.config.clang:
   self.config.available_features.add('clang')
   builtin_include_dir = self.get_clang_builtin_include_dir(
@@ -569,11 +575,12 @@
 (' %clang-cl ',
  '''\"*** invalid substitution, use '%clang_cl'. ***\"'''))
 
-def use_lld(self, additional_tool_dirs=[], required=True):
+def use_lld(self, additional_tool_dirs=[], required=True,
+use_installed=False):
 """Configure the test suite to be able to invoke lld.
 
 Sets up some environment variables important to lld, locates a
-just-built or installed lld, and add a set of standard
+just-built or optionally an installed lld, and add a set of standard
 substitutions useful to any test suite that makes use of lld.
 
 """
@@ -593,12 +600,16 @@
 
 self.with_environment('LD_LIBRARY_PATH', paths, append_path=True)
 
-# Discover the 'clang' and 'clangcc' to use.
+# Discover the LLD executables to use.
 
-ld_lld = self.use_llvm_tool('ld.lld', required=required)
-lld_link = self.use_llvm_tool('lld-link', required=required)
-ld64_lld = self.use_llvm_tool('ld64.lld', required=required)
-wasm_ld = self.use_llvm_tool('wasm-ld', required=required)
+ld_lld = self.use_llvm_tool('ld.lld', required=required,
+use_installed=use_installed)
+lld_link = self.use_llvm_tool('lld-link', required=required,
+  use_installed=use_installed)
+ld64_lld = self.use_llvm_tool('ld64.lld', required=required,
+  use_installed=use_installed)
+wasm_ld = self.use_llvm_tool('wasm-ld', required=required,
+ use_installed=use_installed)
 
 was_found = ld_lld and lld_link and ld64_lld and wasm_ld
 tool_substitutions = []
Index: lldb/test/Shell/helper/toolchain.py
===
--- lldb/test/Shell/helper/toolchain.py
+++ lldb/test/Shell/helper/toolchain.py
@@ -147,14 +147,14 @@
 
 llvm_config.use_clang(additional_flags=['--target=specify-a-target-or-use-a-_host-substitution'],
   additional_tool_dirs=additional_tool_dirs,
-   

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

[James Henderson via Phabricator via lldb-commits]

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 added projects: LLDB, LLVM.

This patch stops lit from looking on the PATH for clang, lld and other users of 
use_llvm_tool (currently only the debuginfo-tests) unless the call explicitly 
requests to opt into using the PATH. When not opting in, tests will only look 
in the build directory.

See the mailing list thread starting from 
https://lists.llvm.org/pipermail/llvm-dev/2021-May/150421.html.

`use_clang` is used from:

- The main clang tests (we don't want to test the installed version here, only 
the built version).
- The debuginfo tests (this one is possibly debatable, but the conversation in 
D95339  suggests we should only use the build 
version in a monorepo setting; see also D96511 
).
- The clangd testing (although rGc29513f7e023f125c6d221db179dc40b79e5c074 
 suggests 
clang is not actually used).
- The lldb shell testing (this appears to deliberately allow using the 
installed version).

`use_lld` is used from:

- The main lld tests (we don't want to test the installed version here, only 
the built version).
- The lldb shell testing (this appears to deliberately allow using the 
installed version).

`use_llvm_tool` is used from:

- `use_lld` and `use_clang`
- The debuginfo_tests to lookup LLDB (see above reasoning for clang)
- The tests for lit itself (not in a way where the behaviour change is relevant)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102630

Files:
  lldb/test/Shell/helper/toolchain.py
  llvm/utils/lit/lit/llvm/config.py

Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -401,10 +401,11 @@
 
 self.add_err_msg_substitutions()
 
-def use_llvm_tool(self, name, search_env=None, required=False, quiet=False):
+def use_llvm_tool(self, name, search_env=None, required=False, quiet=False,
+  use_installed=False):
 """Find the executable program 'name', optionally using the specified
-environment variable as an override before searching the
-configuration's PATH."""
+environment variable as an override before searching the build directory
+and then optionally the configuration's PATH."""
 # If the override is specified in the environment, use it without
 # validation.
 tool = None
@@ -412,7 +413,11 @@
 tool = self.config.environment.get(search_env)
 
 if not tool:
-# Otherwise look in the path.
+# Use the build directory version.
+tool = lit.util.which(name, self.config.llvm_tools_dir)
+
+if not tool and use_installed:
+# Otherwise look in the path, if enabled.
 tool = lit.util.which(name, self.config.environment['PATH'])
 
 if required and not tool:
@@ -429,11 +434,11 @@
 return tool
 
 def use_clang(self, additional_tool_dirs=[], additional_flags=[],
-  required=True):
+  required=True, use_installed=False):
 """Configure the test suite to be able to invoke clang.
 
 Sets up some environment variables important to clang, locates a
-just-built or installed clang, and add a set of standard
+just-built or optionally an installed clang, and add a set of standard
 substitutions useful to any test suite that makes use of clang.
 
 """
@@ -497,7 +502,8 @@
 
 # Discover the 'clang' and 'clangcc' to use.
 self.config.clang = self.use_llvm_tool(
-'clang', search_env='CLANG', required=required)
+'clang', search_env='CLANG', required=required,
+use_installed=use_installed)
 if self.config.clang:
   self.config.available_features.add('clang')
   builtin_include_dir = self.get_clang_builtin_include_dir(
@@ -569,11 +575,12 @@
 (' %clang-cl ',
  '''\"*** invalid substitution, use '%clang_cl'. ***\"'''))
 
-def use_lld(self, additional_tool_dirs=[], required=True):
+def use_lld(self, additional_tool_dirs=[], required=True,
+use_installed=False):
 """Configure the test suite to be able to invoke lld.
 
 Sets up some environment variables important to lld, locates a
-just-built or installed lld, and add a set of standard
+just-built or optionally an installed lld, and add a set of standard
 substitutions useful to any test suite that makes use of lld.
 
 """
@@ -593,12 +60

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

[James Henderson via Phabricator via lldb-commits]

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',
-  'check-debuginfo', 'check-lldb', '-k2'])
+  'check-cross_project-debuginfo-tests', 
'check-lldb', '-k2'])
 footer()

Either I made a typo somewhere, or my comments were unclear apparently. This 
should be one of:

`check-cross-project`

or

`check-debuginfo`

There is no need to change it from what was there before, if you wish to 
maintain the behaviour as it was. If you change it to `check-cross-project`, 
additional tests may start getting picked up as they are added that aren't 
specifically debug info tests.


Repository:
  rZORG LLVM Github Zorg

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105134/new/

https://reviews.llvm.org/D105134

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 cannot skip it or 
make
+// any reasonable assumptions.

labath wrote:
> BTW, I think this error should be recoverable too. I believe the reason why 
> the length field comes *before* the version number is specifically so that 
> one can skip over contributions with unsupported (future) version numbers.
> 
> While it's hard to say what the future versions of dwarf will look like, I 
> would expect that the committee will try very hard to avoid  making changes 
> in the length field. I think they'd use one of the 
> DW_LENGTH_lo_reserved..DW_LENGTH_hi_reserved-1 constants for severely 
> incompatible changes.
"Unrecoverable" here means don't try to parse this table, but do allow parsing 
the next. I think the comment might be slightly misleading in this regard. 
FWIW, a version of 0 or 1 probably doesn't have a leading length, so it is 
definitely unrecoverable. For versions > 5, which are now checked, we don't 
know what the structure of the header is, so although we could take a guess, 
we'd almost certainly get it wrong and produce invalid (possibly very invalid) 
output. I don't have a strong opinion as to whether that should be an 
unrecoverable error or not (currently it is).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72158/new/

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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. I was thinking of a different code base.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 `Optional`?
> `AttrType` is the enum in `ELFAttrs`. There is also `AttrType` in 
> `ARMBuildAttributes` and `RISCVAttributes`. They all could be implicitly 
> converted to integer. So, I use integer as the common interface between 
> different architecture attributes enum.
Okay, seems reasonable. I wonder if AttrType needs to be specified as having 
`unsigned` underlying tap (in all cases) to be consistent with the 
`TagNameItem::attr` member.



Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:1
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"

Missing licence header.

Test files usually are called *Test.cpp, where * is the base file/class that is 
being tested. It seems like this file is only testing the ARMAttributeParser. 
Should it be called "ARMAttributeParserTest.cpp"



Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:5
+#include "gtest/gtest.h"
+#include 
+

I think it's normal to put a blank line between standard library headers and 
other includes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 implementation of package files defined a number of section
+// identifiers with values that clash definitions in the DWARFv5 standard.

I'd guess this should probably use doxygen-style comments?

Same goes for below.



Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:66
+// pre-standard GNU proposal or 5 for DWARFv5 package file.
+uint32_t SerializeSectionKind(DWARFSectionKind Kind, unsigned IndexVersion);
+

lowerCamelCase for function names. Same below.



Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:116
+  // This is a parallel array of raw section IDs for columns of unknown kinds.
+  // This array is created only if there are items in columns ColumnKinds with
+  // DW_SECT_EXT_unknown and the only initialized items here are those with

"items in columns ColumnKinds" doesn't read well to me. I'm not sure if its 
missing punctuation, an extra word, or what.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:101
+if (Version != 5)
+  return false;
+*OffsetPtr += 2; // Skip padding.

Probably out of scope for this change, but this should return an llvm::Error 
instead to say why parsing failed.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:131
 
+  // Fix InfoColumnKind: in DWARFv5, type units also lay in .debug_info.dwo.
+  if (Header.Version == 5)

also lay -> are



Comment at: llvm/test/DebugInfo/X86/dwp-v2-cu-index.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-cu-index - | \

Might be worth a brief comment at the top of this test saying this is the 
pre-standard GCC version.



Comment at: llvm/test/DebugInfo/X86/dwp-v2-loc.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-info -debug-loc - | \

I might have missed something, but is this relevant? I thought this patch was 
for supporting .debug_cu_index?



Comment at: llvm/test/DebugInfo/X86/dwp-v2-tu-index.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-tu-index - | \

Add a comment again about "v2".



Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:614
+if (CUIndex.getVersion() != 2)
+  return make_error("Unsupported cu_index version");
 

I see the above error message starts with a capital letter, but more generally 
I think we try to use lower-case for error messages. Maybe worth doing it right 
here and changing the above line in a separate change?



Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:653
+  if (TUIndex.getVersion() != 2)
+return make_error("Unsupported tu_index version");
   addAllTypesFromDWP(

Same comment as above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75929/new/

https://reviews.llvm.org/D75929



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 unclear why that value is the correct one. Is 
there another way of writing this that would be more correct (e.g. bitwise `&` 
against a known flag value)?



Comment at: llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test:20
+
+# DISASM:mul a0, a1, a2
+

No need for the large amount of whitespace between `DISASM:` and `mul`. One 
space is sufficient.



Comment at: llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test:33
+## The content is the encoding of "mul a0, a1, a2".
+## The encoding could be decoded only when M-extension is enabled.
+Content: 3385C502

when the "m" extension



Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:8
+//===--===//
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"

I think it's normal to have a blank line after the licence header.



Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:17
+static void testParseError(ArrayRef bytes, const char *msg) {
+  ARMAttributeParser parser;
+  Error e = parser.parse(bytes, support::little);

If these tests are for the generic parts of the attribute parser, you should 
probably define a concrete parser type that is neither ARM nor RISC-V, and use 
that in the tests, e.g. `TestAttributeParser`.

If that's not practical for whatever reason, you need to put a comment 
somewhere in this file indicating that the `ARMAttributeParser` is used here 
for generality. Alternatively, consider changing this function to take a 
template argument for the parser type, and call the test for all instantiated 
parser types, or simply duplicating the contents of this function, but for the 
other parser type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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: llvm/test/DebugInfo/X86/dwp-v5-tu-index.s:1
+## The test checks that we can parse and dump a TU index section that compliant
+## to the DWARFv5 standard.

that is compliant



Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:614
+if (CUIndex.getVersion() != 2)
+  return make_error("Unsupported cu_index version");
 

jhenderson wrote:
> I see the above error message starts with a capital letter, but more 
> generally I think we try to use lower-case for error messages. Maybe worth 
> doing it right here and changing the above line in a separate change?
Ping?



Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:653
+  if (TUIndex.getVersion() != 2)
+return make_error("Unsupported tu_index version");
   addAllTypesFromDWP(

jhenderson wrote:
> Same comment as above.
Ping?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75929/new/

https://reviews.llvm.org/D75929



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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, bool &handled) {
+// Dummy function. There is no attribute need to be handled.
+handled = true;

I might suggest changing this comment to "Treat all attributes as handled."



Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:27
+public:
+  AttributeHeaderParser(ScopedPrinter *sw)
+  : ELFAttributeParser(sw, emptyTagNameMap, "test") {}

`sw` seems a bit of a random name. What does it mean?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 bot? They look like they could be related to this somehow.
>
>
> @jhenderson, yes, I found test failures in harbormaster. The failures are 
> occurred after I rebased my patch on master branch. After digging into error 
> messages, I found the failures are triggered by find_if(). Maybe I misuse 
> find_if() in this patch? Do you have any idea about this?
>  By the way, I also found some patch, D75015 
> , landed even harbormaster is failed. I am 
> curious about is it a necessary condition to pass harbormaster before landing?


I don't have much understanding of how Harbormaster works, and it may be that 
the failures are unrelated to anything you did, since I believe it just applies 
your patch on top of the current HEAD of master, which might not work for 
various reasons. Still, it's worth reviewing and locally checking the same 
tests to make sure they aren't failing locally. If you review the logs 
produced, you might spot an issue. If Harbormaster is failing for a reason 
related to your patch, your patch will almost certainly cause build bot 
failures, so in that case, it is necessary to fix the issues (but in other 
cases, if the issues are unrelated, it isn't).

As for why find_if isn't working, I don't know, and I'd suggest you debug it.




Comment at: llvm/include/llvm/Support/ARMBuildAttributes.h:37
   File  = 1,
   CPU_raw_name  = 4,
   CPU_name  = 5,

Same comment as elsewhere.



Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:15
   { ARMBuildAttrs::File, "Tag_File" },
   { ARMBuildAttrs::Section, "Tag_Section" },
   { ARMBuildAttrs::Symbol, "Tag_Symbol" },

By the way: this clang-format failure might be related to your changes, so it's 
probably worth checking to see if it is incorrect without your changes, and if 
not, reformat it as part of this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 bits.




Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:66-68
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags(tagData,
+   sizeof(tagData) /
+   sizeof(TagNameItem));

clang-format appears to be complaining here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 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 and leave the 
> > minimum to `test/llvm-readobj/ELF/{ARM,RISCV}/`
>
>
> Agree. I will remove redundant tests in 
> test/tools/llvm-readobj/ELF/{ARM,RISCV}/.


I also agree with testing primarily through the unit-tests where possible. 
However, there are probably aspects that still need to be tested via lit, 
namely code behaviour in ELFDumper.cpp, so just be slightly wary. High-level 
examples that might or might not be relevant for this patch are making sure 
that the output is printed correctly, and making sure errors/warnings can be 
threaded up to the final output.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 need the `readlines()` part of this line - my understanding 
is that you can iteratre over a file and it'll yield a line per iteration.



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)

You an probably omit the first argument here, as demonstrated. That will split 
on the first sequence of whitespace.



Comment at: llvm/utils/lit/lit/cl_arguments.py:154
 selection_group.add_argument("--shuffle",
-help="Run tests in random order",
+help="Start tests in random order",
 action="store_true")

I'd keep the old wording here and below (specifically "Start" -> "Run"). 
"Start" implies the tests might be started in a random order, but it may change 
mid run, which doesn't really make sense.



Comment at: llvm/utils/lit/lit/cl_arguments.py:214
+if opts.incremental:
+print('WARNING: --incremental was ignored. Failing tests now always 
start first.')
+

Maybe rather than "was ignored" use "is deprecated". Also "start" -> "run" as 
before.



Comment at: llvm/utils/lit/lit/main.py:175
+else:
+assert order == TestOrder.DEFAULT
+tests.sort(key=lambda t: (not t.previous_failure, -t.previous_elapsed, 
t.getFullName()))

Perhaps worth a message with this assert to make it clear that the failure is 
due to an unknown TestOrder value.



Comment at: llvm/utils/lit/lit/main.py:273-274
+for s, value in times_by_suite.items():
+try:
+  path = os.path.join(s, '.lit_test_times.txt')
+  with open(path, 'w') as time_file:

Indentation here is inconsistent.



Comment at: llvm/utils/lit/tests/reorder.py:1
+## Check that we can reorder test starts
+

I don't really understand what you mean by "starts" here. Do you mean something 
like "Check that we can change the test order."

Also, missing full stop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98179/new/

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 failing tests, so being able to distinguish between those 
tests that failed and those that passed in the previous run would basically 
solve this (after a new option has been added to lit). When I say "failed" 
here, I actually mean XPASSes and failures that weren't XFAILs (plus UNRESOLVED 
etc). Running a subset of the lit testsuite would augment these results, rather 
than trash them (i.e. tests outside the subset would still be considered as 
failing until such time as they get run and then pass). I'm not asking for this 
retry mechanism to be implemented as part of that, but if this change can be 
made to work such that the retry feature is able to easily build on top of 
that, that would be great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98179/new/

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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: llvm/utils/lit/lit/main.py:275
+  path = os.path.join(s, '.lit_test_times.txt')
+  with open(path, 'w') as file:
+  for name, time in value:

`file` is a builtin python type, so don't use it as a variable name. Just call 
it `time_file` or something like that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98179/new/

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 wrote:
> You an probably omit the first argument here, as demonstrated. That will 
> split on the first sequence of whitespace.
There's a typo in my example: it should be `maxsplit=1` (with the `s`). You 
need to use the argument name as shown. Example usage from my local python 
instance:
```
>>> s = '123 456 789'
>>> s.split(maxsplit=1)
['123', '456 789']
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98179/new/

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 cherry-pick it to Swift's LLVM branch. Thanks for all the feedback so 
> far.

LGTM, but probably best to get someone else to confirm they're happy too.




Comment at: llvm/utils/lit/lit/cl_arguments.py:159-161
+#selection_group.add_argument("--only-failures",
+#help="Only run tests that failed the previous testing cycle",
+#action="store_true")

Please delete this before pushing.



Comment at: llvm/utils/lit/lit/main.py:74-75
 
+#if opts.only_failures:
+#selected_tests = [t for t in selected_tests if t.previous_failure]
+

Please delete this before pushing.



Comment at: llvm/utils/lit/tests/shtest-shell.py:11
 # on stdout-encoding.txt
+# FIXME: lit's testing sets source_root == exec_root which complicates running 
lit more than once per test
+# RUN: rm -f %{inputs}/shtest-shell/.lit_test_times.txt

Nit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98179/new/

https://reviews.llvm.org/D98179

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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.




Comment at: llvm/include/llvm/Support/MemoryBuffer.h:78-80
   /// if successful, otherwise returning null. If FileSize is specified, this
   /// means that the client knows that the file exists and that it has the
   /// specified size.

You need to remove the reference to FileSize from the description here.



Comment at: llvm/lib/Support/MemoryBuffer.cpp:108
 static ErrorOr>
 getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize,
+   uint64_t Offset, bool IsText, bool RequiresNullTerminator,

Are there users of the `FileSize` here? If not, can you repeat the change for 
this function?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99182/new/

https://reviews.llvm.org/D99182

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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,
+  /*RequiresNullTerminator=*/false, IsVolatile);

Whilst you're modifying, add the named parameter comment for the `0`, so that 
it's not just a magic number.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99182/new/

https://reviews.llvm.org/D99182

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 failing...)



Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:748-750
+##   ^---   4-byte 
(accumulated length 0x04)
+##^---  4-byte 
(accumulated length 0x08)
+## ^--- 4-byte 
(accumulated length 0x0c)

I don't think these comments are particularly helpful, since they can be 
inferred from the byte-count/offset at the start of the line. I think if you 
want comments, you should say what the bytes represent, like you do elsewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84008/new/

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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/

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:380-382
+  cantFail(writeVariableSizedInteger(Unit.AbbrOffset,
+ Unit.Format == dwarf::DWARF64 ? 8 : 4,
+ OS, DI.IsLittleEndian));

labath wrote:
> a writeDwarfOffset function might be handy.
I'm guessing the new function can be used elsewhere too (I expect in places 
like .debug_aranges for example). In another patch, please consider looking at 
that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84008/new/

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 draw in 
one or two others? @JDevlieghere / @labath, any thoughts?


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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.



Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34
+private:
+  Data &DWARF;
+  std::map AbbrevID2Index;

Would it make any sense to merge the `DWARFYAML::Data` class and 
`DWARFYAML::DWARFState` class at all?



Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:35
+  Data &DWARF;
+  std::map AbbrevID2Index;
+

Can you use `std::unordered_map` here? Do we need deterministic ordering? Might 
also be worth naming it something like `AbbrevTableID2Index` to avoid confusing 
with the `AbbrevCode` values.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:92
+  for (uint64_t I = 0; I < DI.DebugAbbrev.size(); ++I) {
+// If the abbrev table's ID isn't specified, we use the index as its ID.
+uint64_t ID = I;

Maybe to avoid weird errors, it might make sense to disallow mixing the two 
methods, i.e. if one table has an explicit ID, the others all must do too. What 
do you think? It may not be worth it if the code is more complex, I don't know.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:105
+}
+AbbrevID2Index.insert({ID, I});
+  }

Could you use the return value of `insert` to identify whether the key already 
exists in the map? That way, you don't need the explicit count call.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:306-313
+  Optional AbbrevTableIndex =
+  DS.getAbbrevTableIndexByID(AbbrevTableID);
+  if (!AbbrevTableIndex)
+return createStringError(errc::invalid_argument,
+ "cannot find abbrev table whose ID is " +
+ utostr(AbbrevTableID));
+  ArrayRef AbbrevDecls(

It might make more sense to do this work in the caller of this function, and to 
maintain this function's interface.



Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:271
+  // There might be DWARF sections and they might be interlinked. We use
+  // DWARFState to get them interlinked.
+  Optional DWARFState;





Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:679-681
+## c) Test that yaml2obj is able to generate compilation units according to the
+## associated abbrev table that is referenced by the 'AbbrevTableID' and 
obj2yaml
+## is able to convert it back.

Is there any particular reason you don't have something equivalent to this in 
the ELF testing, to show you can have tables out-of-order etc.



Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:711
+# MULTI-TABLES-NEXT:   Form:  DW_FORM_udata
+# MULTI-TABLES-NEXT:   debug_info:
+# MULTI-TABLES-NEXT: - Length:0x000C

Something's not right here - the YAML below has four debug_info tables, but 
only three have check lines. What happened to the one that should be 
referencing abbrev table 2?



Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:873-884
+  - sectname:  __debug_abbrev
+segname:   __DWARF
+addr:  0x00
+size:  24
+offset:528
+align: 0
+reloff:0x

Can you omit the abbrev section entirely?



Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:906-911
+## e) Test that yaml2obj emits an error message when multiple abbrev tables 
are assigned
+## the same ID.
+
+# RUN: not yaml2obj --docnum=5 %s 2>&1 | FileCheck %s 
--check-prefix=ID-COLLISION
+
+# ID-COLLISION: yaml2obj: error: the ID (1) of abbrev table with index 1 has 
been used

This feels like it belongs in a debug_abbrev dedicated test?



Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:924-925
+
+## o) Test that yaml2obj emits an error message when multiple abbrev tables 
are assigned
+## the same ID.
+

Same as Mach-O, this really is a debug_abbrev test, not a debug_info test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83116/new/

https://reviews.llvm.org/D83116

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:100
+  for (const DWARFYAML::AbbrevTable &AbbrevTable : DI.DebugAbbrev) {
+for (auto AbbrevDecl : AbbrevTable.Table) {
+  AbbrevCode =

Don't use `auto` here. It's not obvious what the type is.



Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml:900
+
+## n) Test that yaml2obj emits an error message when a compilation unit has 
values but there is no associated abbrev tables.
+





Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:26-43
+  DWARFYAML::AbbrevTable AbbrevTable;
   for (auto AbbrvDecl : AbbrvDeclSet.second) {
 DWARFYAML::Abbrev Abbrv;
 Abbrv.Code = AbbrvDecl.getCode();
 Abbrv.Tag = AbbrvDecl.getTag();
 Abbrv.Children = AbbrvDecl.hasChildren() ? dwarf::DW_CHILDREN_yes
  : dwarf::DW_CHILDREN_no;

Does this work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86194/new/

https://reviews.llvm.org/D86194

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 yaml2obj is able to emit the __debug_abbrev section and obj2yaml 
is able to
+## convert it back.

Let's move this test change into a different patch, to avoid doing too much at 
once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86194/new/

https://reviews.llvm.org/D86194

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.



Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34
+private:
+  Data &DWARF;
+  std::map AbbrevID2Index;

Higuoxing wrote:
> labath wrote:
> > jhenderson wrote:
> > > Would it make any sense to merge the `DWARFYAML::Data` class and 
> > > `DWARFYAML::DWARFState` class at all?
> > That would definitely be nice.
> >>! @jhenderson :
> > Would it make any sense to merge the `DWARFYAML::Data` class and 
> > `DWARFYAML::DWARFState` class at all?
> 
> >>! @labath :
> > That would definitely be nice.
> 
> I think they can be merged. But is there any particular reason to merge them? 
> Personally, I would like to keep the `DWARFYAML::DWARFState` class. Here are 
> a few concerns I would like to address:
> 
> - If we merge the `DWARFYAML::DWARFState` and `DWARFYAML::Data` at all, we 
> will need to add some helper variables, e.g. `AbbrevTableID2Index` and a 
> method like `Error DWARFYAML::link()` to `DWARFYAML::Data` class to link 
> DWARF sections. We will have to call this special method before DWARF 
> sections being emitted. If we port DWARF support to other binary format in 
> the future, e.g., wasm, we might forget to call this method. If we make DWARF 
> emitters accept `DWARFYAML::DWARFState`, it's easy to figure out that we need 
> a `DWARFState` before emitting DWARF sections.
> 
> - Besides, I think `DWARFYAML::Data` and `DWARFYAML::DWARFState` have 
> different functions. `DWARFYAML::Data` is used to contain original DWARF 
> section descriptions and `DWARFYAML::DWARFState` contains helper structs and 
> methods which helps link DWARF sections. It might be good not to merge them 
> so that we can easily figure out when and where are those sections get linked?
> 
> I can be persuaded :-)
> I think they can be merged. But is there any particular reason to merge them?
As things stand, it mostly seems like `DWARFState` merely gets in the way of 
accessing the `Data` class. It seems to me like it would be easier to use if 
the two were one class, as you wouldn't have to keep going through an extra 
function call/member access.


> - If we merge the `DWARFYAML::DWARFState` and `DWARFYAML::Data` at all, we 
> will need to add some helper variables, e.g. `AbbrevTableID2Index` and a 
> method like `Error DWARFYAML::link()` to `DWARFYAML::Data` class to link 
> DWARF sections. We will have to call this special method before DWARF 
> sections being emitted. If we port DWARF support to other binary format in 
> the future, e.g., wasm, we might forget to call this method. If we make DWARF 
> emitters accept `DWARFYAML::DWARFState`, it's easy to figure out that we need 
> a `DWARFState` before emitting DWARF sections.
If, rather than do all the calculations up front (e.g. mapping tables to IDs), 
you put the `Data` members behind getters, you could then add the "linking" 
code there. For example, with the AbbrevTable ID mapping, you could have the 
`getAbbrevTableIndexByID` method, which is the only way of getting an abbrev 
table from outside the class.  The first time this is called (or optionally 
every time), it works out the mapping between ID(s) and table(s). This avoids 
the need for an explicit `link` or `finalize` method.

> - Besides, I think `DWARFYAML::Data` and `DWARFYAML::DWARFState` have 
> different functions. `DWARFYAML::Data` is used to contain original DWARF 
> section descriptions and `DWARFYAML::DWARFState` contains helper structs and 
> methods which helps link DWARF sections. It might be good not to merge them 
> so that we can easily figure out when and where are those sections get linked?
I'm not sure I agree they have different functions. The `AbbrevID2Index` map is 
just a helper variable that is directly derived from the `Data` struct's 
members. We could get rid of it entirely, and just move the 
`getAbbrevTableIndexByID` method into `Data`, rewriting it to just run through 
the list of tables each time to find the right ID. This is obviously less 
efficient than instantiating a mapping up front, if done repeatedly, but I 
think it indicates that this extra layer is not actually doing anything 
distinct. Similar principles apply probably for other things, although without 
further concrete examples, it's hard to discuss them!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83116/new/

https://reviews.llvm.org/D83116

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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);
+
+private:
+  std::unordered_map AbbrevTableID2Index;

Something to consider here: the user-visible behaviour of this class is that 
this is a `const` operation, but because this method causes 
`AbbrevTableID2Index` to be populated on the first call, it technically isn't. 
I would say this is a reasonable use case for `mutable` - the cache (in this 
case `AbbrevTableID2Index`) is marked as mutable, and then you can continue to 
pass this class around as `const`.



Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:92
+  for (uint64_t I = 0; I < DI.DebugAbbrev.size(); ++I) {
+// If the abbrev table's ID isn't specified, we use the index as its ID.
+uint64_t ID = I;

Higuoxing wrote:
> jhenderson wrote:
> > Maybe to avoid weird errors, it might make sense to disallow mixing the two 
> > methods, i.e. if one table has an explicit ID, the others all must do too. 
> > What do you think? It may not be worth it if the code is more complex, I 
> > don't know.
> I think it's a little bit difficult to distinguish these 2 situations. 
> Besides, sometimes we might want to add one compilation unit to a test case 
> and let it reference an existing abbrev table. We don't need to mutate the 
> whole test case to add IDs. What do think of it?
Leave it as is. I wasn't convinced by my own statement, so I think what you've 
got is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83116/new/

https://reviews.llvm.org/D83116

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 number of abbrev tables
+

Higuoxing wrote:
> jhenderson wrote:
> > Perhaps worth including the index and table count values in this error 
> > message to make it easier for people to identify the problem.
> I've included the abbrev table ID and compilation unit count values in test 
> case (n). Can we prettify the error message in test case (i) in a follow-up 
> patch?
That's fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83116/new/

https://reviews.llvm.org/D83116

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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.




Comment at: lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:53
+DebugAbbrevOffset: 0
+AddressSize:8
 Entries:

Nit: the value here is misaligned.



Comment at: llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp:2296
 
 
   StringRef yamldata = R"(

Could you fix this linter nit, whilst you're here, please?


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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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

https://reviews.llvm.org/D87878

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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: include/llvm/BinaryFormat/Minidump.h:81
+#include "llvm/BinaryFormat/MinidumpConstants.def"
+  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue = */ 0xu),
+};

I believe if you format this line as:


```
LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xu),
```

clang-format will leave it unedited. I believe it has special rules for 
`/*=*/` to label parameters.



Comment at: lib/Object/Minidump.cpp:58
+MinidumpFile::getMemoryInfoList() const {
+  auto OptionalStream = getRawStream(StreamType::MemoryInfoList);
+  if (!OptionalStream)

I probably should have picked up on this in previous reviews, but this is too 
much `auto` for my liking, as it's not obvious from the call site what 
`getRawStream` returns.



Comment at: lib/Object/Minidump.cpp:66
+  const minidump::MemoryInfoListHeader &H = ExpectedHeader.get()[0];
+  auto ExpectedData = getDataSlice(*OptionalStream, H.SizeOfHeader,
+   H.SizeOfEntry * H.NumberOfEntries);

Ditto.



Comment at: unittests/Object/MinidumpTest.cpp:617-618
+  // MemoryInfoListHeader
+  16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+  1, 0, 0, 0,   // ???
+  };

I might make the data here be of size 15 to test the edge case.

It's probably also worth a test case where the header size as specified by 
SizeOfHeader fits in the data but is smaller than the expected value.



Comment at: unittests/Object/MinidumpTest.cpp:634
+  // MemoryInfoListHeader
+  16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+  1, 0, 0, 0, 0, 0, 0, 0,   // NumberOfEntries

I might go for a value of 49 to test the edge value here.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68210/new/

https://reviews.llvm.org/D68210



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 have picked up on this in previous reviews, but this is 
> > too much `auto` for my liking, as it's not obvious from the call site what 
> > `getRawStream` returns.
> Done. I've also changed the other calls to getRawStream.
Thanks!



Comment at: unittests/Object/MinidumpTest.cpp:620
+  };
+  EXPECT_THAT_EXPECTED(cantFail(create(HeaderTooBig))->getMemoryInfoList(),
+   Failed());

Here and in the similar places, I'm not convinced that `cantFail` is 
appropriate (if the creation code is broken, this will assert and therefore 
possibly hide the actual testing failures that show where it went wrong more 
precisely). It should probably be a two phase thing:

```
Expected> Minidump = HeaderTooBig);
ASSERT_THAT_EXPECTED(Minidump, Succeeded());
EXPECTE_THAT_EXPECTED(Minidump->getMemoryInfoList(), Failed());
```



Comment at: unittests/Object/MinidumpTest.cpp:624
+  // Header fits into the stream, but it is too small to contain the required
+  // entries).
+  std::vector HeaderTooSmall{

Nit: delete the ')'


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68210/new/

https://reviews.llvm.org/D68210



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 add @grimar and 
@MaskRay to the reviews as they've been doing a lot of work in 
obj2yaml/yaml2obj recently.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68645/new/

https://reviews.llvm.org/D68645



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 module list to
-  // 8-byte boundary. Check for that by comparing the module list size with the
-  // overall stream size.
-  if (ListOffset + sizeof(Module) * ListSize < OptionalStream->size())
+  // Some producers insert additional padding bytes to align the list to 8-byte
+  // boundary. Check for that by comparing the list size with the overall 
stream

Nit (was there before): to 8-byte -> to an 8-byte



Comment at: unittests/Object/MinidumpTest.cpp:446
+
+  for (const std::vector &Data : {OneThread, PaddedThread}) {
+auto ExpectedFile = create(Data);

I missed this in the other tests, but this could be an `ArrayRef` 
instead of a `const std::vector &`. Feel free to leave it as is or to 
update it in an NFC throughout this test afterwards, if you would prefer to 
leave it to another patch.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61064/new/

https://reviews.llvm.org/D61064



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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: include/llvm/ObjectYAML/MinidumpYAML.h:58
+/// streams with similar structure.
+template 
+struct ListStream : public Stream {

KindV and TypeV aren't clear names to me. What does the V stand for?



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:64
 
-  static bool classof(const Stream *S) {
-return S->Kind == StreamKind::ModuleList;
-  }
+  ListStream(std::vector Entries = {})
+  : Stream(KindV, TypeV), Entries(std::move(Entries)) {}

`explicit`?



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70-73
+/// A structure containing all data belonging to a single minidump module. On
+/// disk, these are placed at various places in the minidump file and
+/// cross-referenced via their offsets, but for ease of use, we group them
+/// together in the logical memory view.

I'm not sure how much sense it makes to go into the detail of the minidump file 
format versus the memory view here. I also am not convinced by the repetition 
of this in the comments below.



Comment at: lib/ObjectYAML/MinidumpYAML.cpp:481
+
+template 
+static size_t

Same comment as above re. names.



Comment at: test/tools/obj2yaml/basic-minidump.yaml:47-49
+  - Thread Id:   0x5C5D5E5F
+Priority Class:  0x60616263
+Environment Block: 0x6465666768696A6B

It would be nice if these were padded so that they all line up. Ditto in the 
Stack block below.



Comment at: test/tools/obj2yaml/basic-minidump.yaml:51
+Stack:   
+  Start of Memory Range: 0x6C6D6E6F70717273
+  Content: 7475767778797A7B

I don't have a concrete suggestion, but it might be nice to have a shorter 
field name than "Start of Memory Range", but that's less of a concern if that's 
the actual minidump field name.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61423/new/

https://reviews.llvm.org/D61423



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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
+Environment Block: 0x6465666768696A6B

labath wrote:
> jhenderson wrote:
> > It would be nice if these were padded so that they all line up. Ditto in 
> > the Stack block below.
> The microsoft structure definition calls this field just "teb" (for Thread 
> Environment Block), but I've found that too opaque, so I expanded the acronym 
> (sans "thread", because it is obvious we are talking about threads here). I 
> could shorten this further to "environment" (the word "block" probably 
> doesn't add that much value) , or even to "teb" for consistency with 
> microsoft headers. Let me know what you think.
Environment Block is fine. I was actually referring to the number of spaces 
between the attribute name and value, i.e. I'd prefer this:

```
  - Thread Id: 0x5C5D5E5F
Priority Class:0x60616263
Environment Block: 0x6465666768696A6B
```



Comment at: test/tools/obj2yaml/basic-minidump.yaml:51
+Stack:   
+  Start of Memory Range: 0x6C6D6E6F70717273
+  Content: 7475767778797A7B

labath wrote:
> jhenderson wrote:
> > I don't have a concrete suggestion, but it might be nice to have a shorter 
> > field name than "Start of Memory Range", but that's less of a concern if 
> > that's the actual minidump field name.
> That's how the field is called in the official microsoft documentation 
> (https://docs.microsoft.com/en-us/windows/desktop/api/minidumpapiset/ns-minidumpapiset-minidump_memory_descriptor),
>  which is probably the closest thing to a "spec" for this thing. It's a bit 
> verbose, and probably "Address" would just suffice here, but otoh it's nice 
> for this to match the official name. 
Let's leave it as is, since it matches the Microsoft document.



Comment at: test/tools/obj2yaml/basic-minidump.yaml:105
+# CHECK-NEXT:   Start of Memory Range: 0x6C6D6E6F70717273
+# CHECK-NEXT:   Content: 7475767778797A7B
+# CHECK-NEXT: Context: 7C7D7E7F80818283

Similar comment here about whitespace. Make the values of attributes within a 
block line up.



Comment at: test/tools/obj2yaml/basic-minidump.yaml:106
+# CHECK-NEXT:   Content: 7475767778797A7B
+# CHECK-NEXT: Context: 7C7D7E7F80818283
 # CHECK-NEXT: ...

Move this to be above the Stack block for readability.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61423/new/

https://reviews.llvm.org/D61423



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 consistency of the Thread entries themselves is 
not
+  /// checked in any way.

These two lines talk about threads. Is that a copy/paste error?



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:92
+/// A structure containing all data describing a single memory region.
+struct ParsedMemoryRange {
+  static constexpr Stream::StreamKind Kind = Stream::StreamKind::MemoryList;

Would it make more sense to call this ParsedMermoryList, to match the 
StreamType?



Comment at: test/tools/obj2yaml/basic-minidump.yaml:54-57
+  - Type:MemoryList
+Memory Ranges:   
+  - Start of Memory Range: 0x7C7D7E7F80818283
+Content: '8485868788'

I'd probably find this neater if the Indentation of values for each entry were 
more consistent, but I'm not too fussed.

Also, in the ThreadList above, the Content is not quoted, but here it is. 
Please standardise it on one or the other.



Comment at: unittests/Object/MinidumpTest.cpp:483
+  };
+  // Same as before, but with a padded thread list.
+  std::vector PaddedRange{

thread? Copy/paste error?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61885/new/

https://reviews.llvm.org/D61885



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, though I haven't bothered to inspect the detail of the minidump format. 
I'll leave it up to you whether you think it's worth getting a separate pair of 
eyes.




Comment at: test/tools/obj2yaml/basic-minidump.yaml:54-57
+  - Type:MemoryList
+Memory Ranges:   
+  - Start of Memory Range: 0x7C7D7E7F80818283
+Content: '8485868788'

labath wrote:
> jhenderson wrote:
> > I'd probably find this neater if the Indentation of values for each entry 
> > were more consistent, but I'm not too fussed.
> > 
> > Also, in the ThreadList above, the Content is not quoted, but here it is. 
> > Please standardise it on one or the other.
> Done. The different quoting of is actually a relict of how obj2yaml prints 
> BinaryRef values (they omit quotes if the data happens to be contain hex 
> (A-F) characters). Do you think it would be worth making this output more 
> consistent too?
Probably, but that sounds like an unrelated changes.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61885/new/

https://reviews.llvm.org/D61885



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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:  
-#CHECK: TotalLength: 117
-#CHECK:   Version: 4
-#CHECK:   AbbrOffset:  0
-#CHECK:   AddrSize:8
-#CHECK:   Entries: 
-#CHECK: - AbbrCode:0x0001
-#CHECK:   Values:  
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x000C
-#CHECK: - Value:   0x0038
-#CHECK: - Value:   0x
-#CHECK: - Value:   0x0046
-#CHECK: - Value:   0x00010F50
-#CHECK: - Value:   0x0034
-#CHECK: - AbbrCode:0x0002
-#CHECK:   Values:  
-#CHECK: - Value:   0x00010F50
-#CHECK: - Value:   0x0034
-#CHECK: - Value:   0x0001
-#CHECK:   BlockData:   [ 0x56 ]
-#CHECK: - Value:   0x0076
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0060
-#CHECK: - Value:   0x0001
-#CHECK: - AbbrCode:0x0003
-#CHECK:   Values:  
-#CHECK: - Value:   0x0002
-#CHECK:   BlockData:   [ 0x91, 0x78 ]
-#CHECK: - Value:   0x007B
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0060
-#CHECK: - AbbrCode:0x0003
-#CHECK:   Values:  
-#CHECK: - Value:   0x0002
-#CHECK:   BlockData:   [ 0x91, 0x70 ]
-#CHECK: - Value:   0x0080
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0067
-#CHECK: - AbbrCode:0x
-#CHECK:   Values:  
-#CHECK: - AbbrCode:0x0004
-#CHECK:   Values:  
-#CHECK: - Value:   0x0085
-#CHECK: - Value:   0x0005
-#CHECK: - Value:   0x0004
-#CHECK: - AbbrCode:0x0005
-#CHECK:   Values:  
-#CHECK: - Value:   0x006C
-#CHECK: - AbbrCode:0x0005
-#CHECK:   Values:  
-#CHECK: - Value:   0x0071
-#CHECK: - AbbrCode:0x0004
-#CHECK:   Values:  
-#CHECK: - Value:   0x0089
-#CHECK: - Value:   0x0006
-#CHECK: - Value:   0x0001
-#CHECK: - AbbrCode:0x
-#CHECK:   Values:  
+#  DWARF32: debug_info:
+# DWARF32-NEXT:   - Length: 117

Any particular reason you've dropped the `DWARF:` tag? Having it back would 
help make the diff a little simpler, I'd hope.



Comment at: llvm/test/ObjectYAML/MachO/DWARF5-debug_info.yaml:1
-# RUN: yaml2obj %s | obj2yaml | FileCheck %s
+## This file contains test cases for generating .debug_info
+## section in object files from the DWARF entry of Mach-O YAML inputs.

generating -> generating a


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82622/new/

https://reviews.llvm.org/D82622



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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:   
-#CHECK:   debug_info:  
-#CHECK: - Length:  
-#CHECK: TotalLength: 117
-#CHECK:   Version: 4
-#CHECK:   AbbrOffset:  0
-#CHECK:   AddrSize:8
-#CHECK:   Entries: 
-#CHECK: - AbbrCode:0x0001
-#CHECK:   Values:  
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x000C
-#CHECK: - Value:   0x0038
-#CHECK: - Value:   0x
-#CHECK: - Value:   0x0046
-#CHECK: - Value:   0x00010F50
-#CHECK: - Value:   0x0034
-#CHECK: - AbbrCode:0x0002
-#CHECK:   Values:  
-#CHECK: - Value:   0x00010F50
-#CHECK: - Value:   0x0034
-#CHECK: - Value:   0x0001
-#CHECK:   BlockData:   [ 0x56 ]
-#CHECK: - Value:   0x0076
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0060
-#CHECK: - Value:   0x0001
-#CHECK: - AbbrCode:0x0003
-#CHECK:   Values:  
-#CHECK: - Value:   0x0002
-#CHECK:   BlockData:   [ 0x91, 0x78 ]
-#CHECK: - Value:   0x007B
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0060
-#CHECK: - AbbrCode:0x0003
-#CHECK:   Values:  
-#CHECK: - Value:   0x0002
-#CHECK:   BlockData:   [ 0x91, 0x70 ]
-#CHECK: - Value:   0x0080
-#CHECK: - Value:   0x0001
-#CHECK: - Value:   0x0003
-#CHECK: - Value:   0x0067
-#CHECK: - AbbrCode:0x
-#CHECK:   Values:  
-#CHECK: - AbbrCode:0x0004
-#CHECK:   Values:  
-#CHECK: - Value:   0x0085
-#CHECK: - Value:   0x0005
-#CHECK: - Value:   0x0004
-#CHECK: - AbbrCode:0x0005
-#CHECK:   Values:  
-#CHECK: - Value:   0x006C
-#CHECK: - AbbrCode:0x0005
-#CHECK:   Values:  
-#CHECK: - Value:   0x0071
-#CHECK: - AbbrCode:0x0004
-#CHECK:   Values:  
-#CHECK: - Value:   0x0089
-#CHECK: - Value:   0x0006
-#CHECK: - Value:   0x0001
-#CHECK: - AbbrCode:0x
-#CHECK:   Values:  
+#  DWARF32: debug_info:
+# DWARF32-NEXT:   - Length: 117

Higuoxing wrote:
> jhenderson wrote:
> > Any particular reason you've dropped the `DWARF:` tag? Having it back would 
> > help make the diff a little simpler, I'd hope.
> Because the first tag after "DWARF:" is "debug_abbrev". I will bring it back 
> later.
I didn't realise it wasn't on the next line. In that case, I'm happy for it to 
be deleted. Alternatively, you should add it to the DWARF64 case too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82622/new/

https://reviews.llvm.org/D82622



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 table, and it does not have to be a 
> sequentially increasing number (though it could of course be that by default).
>
> The thing I'm trying to achieve is to make the yaml more robust against 
> modifications/simplifications. E.g., it would be nice if deleting an abbrev 
> table does not make all compile units suddenly refer to different tables.  If 
> the ids were present explicitly, compile units would be unaffected by this, 
> and one would get an explicit error message if there was a compile unit left 
> which still referred to the deleted abbrev table.
>
> (This is one of the aspects where an assembler is better than yaml, as 
> symbolic labels in asm have these properties.)


+1 to this idea. By default it could of course be an incremental index, when 
emitting, but in yaml2obj, I see no reason for it to be tied in that way 
necessarily.




Comment at: lldb/unittests/Symbol/Inputs/inlined-functions.yaml:348
+  Version:  4
+  AbbrevTableIndex: 1
+  AbbrOffset:   0

`AbbrevTableIndex` should be an optional value. I think by default it should 
pick the first table (or possibly should pick the Nth table, where N is the 
.debug_info table index). Thus in this and pretty much every other case, you 
should be able to omit the new value.



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 number of abbrev tables
+

Perhaps worth including the index and table count values in this error message 
to make it easier for people to identify the problem.



Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:19
 #include 
+#include 
 

I'm slightly surprised you needed to add this. Did this not compile without it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83116/new/

https://reviews.llvm.org/D83116



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 right fix to make or not, but it does at 
least stop this change breaking the build.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72158/new/

https://reviews.llvm.org/D72158

Files:
  lld/test/ELF/Inputs/undef-bad-debug.s
  lld/test/ELF/undef.s
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -359,10 +359,15 @@
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+
+  checkError(
   {"parsing line table prologue at 0x found an invalid directory "
"or file table description at 0x0014",
-   "failed to parse entry content descriptions because no path was found"});
+   "failed to parse entry content descriptions because no path was found"},
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
@@ -379,14 +384,24 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  --Result.Prologue.PrologueLength;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd - 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
@@ -408,16 +423,29 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  if (Version < 5)
+Result.Prologue.PrologueLength += 2;
+  else
+Result.Prologue.PrologueLength += 1;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
   if (Version < 5)
 --ExpectedEnd;
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd + 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -636,14 +664,15 @@
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 62u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 136u);
   EXPECT_TRUE(Parser.done());
 
+  EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
 }
 
@@ -688,10 +717,11 @@
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
+  EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x unsupported "
  "reserved unit length found 

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 the 
> prologue, whereas previously we would stop straight away. That sounds 
> reasonable if now or in the future we will be able to get some useful 
> information (at least some subset of file names, or line numbers without file 
> names, etc.) out of these kinds of line tables.


Indeed, that's what this change allows: the parser will continue parsing after 
reporting the Errors via the new callback. In cases where it can't (i.e. 
unsupported versions or reserved unit length values), it will stop and return 
the Error (as it previously did, and also did for the now-recoverable Errors). 
For now I've changed LLDB to record both kinds of Errors in the same way as 
they were before, but the recoverable errors do not prevent it subsequently 
calling `ParseSupportFilesFromPrologue`. I could just as easily change it to 
doing what it always did for all cases, namely log the errors and not call 
`ParseSupportFilesFromPrologue`. That's probably the safer approach on further 
reflection, so I'll update the patch tomorrow (I'm just about to leave the 
office for the day), unless someone thinks changing the behaviour is good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72158/new/

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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 mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[James Henderson via Phabricator via lldb-commits]

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&id=241076#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72158/new/

https://reviews.llvm.org/D72158

Files:
  lld/test/ELF/Inputs/undef-bad-debug.s
  lld/test/ELF/undef.s
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -359,10 +359,15 @@
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+
+  checkError(
   {"parsing line table prologue at 0x found an invalid directory "
"or file table description at 0x0014",
-   "failed to parse entry content descriptions because no path was found"});
+   "failed to parse entry content descriptions because no path was found"},
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
@@ -379,14 +384,24 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  --Result.Prologue.PrologueLength;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd - 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
@@ -408,16 +423,29 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  if (Version < 5)
+Result.Prologue.PrologueLength += 2;
+  else
+Result.Prologue.PrologueLength += 1;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
   if (Version < 5)
 --ExpectedEnd;
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd + 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -636,14 +664,15 @@
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 62u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 136u);
   EXPECT_TRUE(Parser.done());
 
+  EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
 }
 
@@ -688,10 +717,11 @@
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
+  EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x unsupported "
  "reserved unit length found of value 0xfff0",
@@ -767,11 +797,12 @@
   generate();
 
   DWARFDebugLine::SectionParser Pa