[Lldb-commits] [lldb] 40aab04 - [test] Migrate -gcc-toolchain with space separator to --gcc-toolchain=

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2021-08-20T15:24:58-07:00
New Revision: 40aab0412fe7a14781e133627c2bb0a22761eac8

URL: 
https://github.com/llvm/llvm-project/commit/40aab0412fe7a14781e133627c2bb0a22761eac8
DIFF: 
https://github.com/llvm/llvm-project/commit/40aab0412fe7a14781e133627c2bb0a22761eac8.diff

LOG: [test] Migrate -gcc-toolchain with space separator to --gcc-toolchain=

Space separated driver options are uncommon but Clang traditionally
did not do a good job. --gcc-toolchain= is the preferred form.

Added: 


Modified: 
clang/test/Driver/fuse-ld.c
clang/test/Driver/gcc-toolchain.cpp
libc/benchmarks/CMakeLists.txt
libcxx/benchmarks/CMakeLists.txt
lldb/packages/Python/lldbsuite/test/make/Android.rules
llvm/docs/HowToCrossCompileBuiltinsOnArm.rst

Removed: 




diff  --git a/clang/test/Driver/fuse-ld.c b/clang/test/Driver/fuse-ld.c
index 74a2e5f4d9654..dfcf31e25ffa6 100644
--- a/clang/test/Driver/fuse-ld.c
+++ b/clang/test/Driver/fuse-ld.c
@@ -60,19 +60,19 @@
 
 // RUN: %clang %s -### -fuse-ld=ld \
 // RUN: -target arm-linux-androideabi \
-// RUN: -gcc-toolchain %S/Inputs/basic_android_tree 2>&1 \
+// RUN: --gcc-toolchain=%S/Inputs/basic_android_tree 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-ANDROID-ARM-LD-TC
 // CHECK-ANDROID-ARM-LD-TC: 
Inputs/basic_android_tree/lib/gcc/arm-linux-androideabi/4.4.3/../../../../arm-linux-androideabi/bin{{/|\\+}}ld
 
 // RUN: %clang %s -### -fuse-ld=bfd \
 // RUN: -target arm-linux-androideabi \
-// RUN: -gcc-toolchain %S/Inputs/basic_android_tree 2>&1 \
+// RUN: --gcc-toolchain=%S/Inputs/basic_android_tree 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=CHECK-ANDROID-ARM-BFD-TC
 // CHECK-ANDROID-ARM-BFD-TC: 
Inputs/basic_android_tree/lib/gcc/arm-linux-androideabi/4.4.3/../../../../arm-linux-androideabi/bin{{/|\\+}}ld.bfd
 
 // RUN: %clang %s -### -fuse-ld=gold \
 // RUN: -target arm-linux-androideabi \
-// RUN: -gcc-toolchain %S/Inputs/basic_android_tree 2>&1 \
+// RUN: --gcc-toolchain=%S/Inputs/basic_android_tree 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=CHECK-ANDROID-ARM-GOLD-TC
 // CHECK-ANDROID-ARM-GOLD-TC: 
Inputs/basic_android_tree/lib/gcc/arm-linux-androideabi/4.4.3/../../../../arm-linux-androideabi/bin{{/|\\+}}ld.gold
 

diff  --git a/clang/test/Driver/gcc-toolchain.cpp 
b/clang/test/Driver/gcc-toolchain.cpp
index 7cdba0841b8cd..e3a1670110a5f 100644
--- a/clang/test/Driver/gcc-toolchain.cpp
+++ b/clang/test/Driver/gcc-toolchain.cpp
@@ -8,7 +8,7 @@
 //
 // Additionally check that the legacy spelling of the flag works.
 // RUN: %clangxx %s -### --target=x86_64-linux-gnu --sysroot= \
-// RUN:   -gcc-toolchain %S/Inputs/ubuntu_14.04_multiarch_tree/usr 
-stdlib=libstdc++ --rtlib=libgcc 2>&1 | \
+// RUN:   --gcc-toolchain=%S/Inputs/ubuntu_14.04_multiarch_tree/usr 
-stdlib=libstdc++ --rtlib=libgcc 2>&1 | \
 // RUN:   FileCheck %s
 //
 // Test for header search toolchain detection.

diff  --git a/libc/benchmarks/CMakeLists.txt b/libc/benchmarks/CMakeLists.txt
index 3eece745096f9..2f06836b0fadd 100644
--- a/libc/benchmarks/CMakeLists.txt
+++ b/libc/benchmarks/CMakeLists.txt
@@ -10,7 +10,7 @@ set(LLVM_LINK_COMPONENTS Support)
 set(GOOGLE_BENCHMARK_TARGET_FLAGS ${BENCHMARK_DIALECT_FLAG})
 if (LIBCXX_BENCHMARK_GCC_TOOLCHAIN)
   set(GOOGLE_BENCHMARK_TARGET_FLAGS
-  -gcc-toolchain ${LIBCXX_BENCHMARK_GCC_TOOLCHAIN})
+  --gcc-toolchain=${LIBCXX_BENCHMARK_GCC_TOOLCHAIN})
 endif()
 string(REPLACE ";" " " GOOGLE_BENCHMARK_TARGET_FLAGS 
"${GOOGLE_BENCHMARK_TARGET_FLAGS}")
 

diff  --git a/libcxx/benchmarks/CMakeLists.txt 
b/libcxx/benchmarks/CMakeLists.txt
index c4b8247da63b1..8e33f8ebcf0cf 100644
--- a/libcxx/benchmarks/CMakeLists.txt
+++ b/libcxx/benchmarks/CMakeLists.txt
@@ -47,7 +47,7 @@ ExternalProject_Add(google-benchmark-libcxx
 set(BENCHMARK_NATIVE_TARGET_FLAGS)
 if (LIBCXX_BENCHMARK_NATIVE_GCC_TOOLCHAIN)
   set(BENCHMARK_NATIVE_TARGET_FLAGS
-  -gcc-toolchain ${LIBCXX_BENCHMARK_NATIVE_GCC_TOOLCHAIN})
+  --gcc-toolchain=${LIBCXX_BENCHMARK_NATIVE_GCC_TOOLCHAIN})
 endif()
 split_list(BENCHMARK_NATIVE_TARGET_FLAGS)
 

diff  --git a/lldb/packages/Python/lldbsuite/test/make/Android.rules 
b/lldb/packages/Python/lldbsuite/test/make/Android.rules
index 7339c22d2e4c4..32f786aa34756 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Android.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Android.rules
@@ -66,8 +66,8 @@ OBJCOPY ?= $(GCC_TOOLCHAIN)/bin/$(TOOL_PREFIX)-objcopy
 ARCHIVER ?= $(GCC_TOOLCHAIN)/bin/$(TOOL_PREFIX)-ar
 
 ifeq "$(findstring clang,$(CC))" "clang"
-   ARCH_CFLAGS += -target $(TRIPLE) -gcc-toolchain $(GCC_TOOLCHAIN)
-   ARCH_LDFLAGS += -target $(TRIPLE) -gcc-toolchain $(GCC_TOOLCHAIN)
+   ARCH_CFLAGS += -target $(TRIPLE) --gcc-toolchain=$(GCC_TOOLCHAIN)
+   ARCH_LDFLAGS += -target $(TRIPLE) --gcc-toolchain=$(GCC_TOOLCHAIN)
 endif
 
 ARCH_CFLAGS += 

[Lldb-commits] [PATCH] D108395: [lldb] Delete IRExecutionUnit::CollectCandidateCPlusPlusNames

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D108395#2957194 , @teemperor wrote:

> I'm very supportive of the idea that we delete everything that isn't tested 
> :) (and I'm only partly joking here).

I can sympathize, I'm only partly serious with this change! :D

> However removing this should cause a bunch of test failures on Linux as it 
> effectively disables D70721  (the test added 
> in that patch was just a unit test that is preserved with this patch, but the 
> bigger problem describes there will break a few tests calling constructors on 
> Linux).

It doesn't cause test any test failures on my Linux machine unfortunately. I 
don't think this codepath is currently exercised by any tests. Certainly the 
`FindAlternateFunctionManglings` test in `CPlusPlusLanguageTest.cpp` also uses 
`CPlusPlusLanguage::FindAlternateFunctionManglings`, but that's not the same 
thing as testing `IRExecutionUnit::CollectCandidateCPlusPlusNames` being tested 
as a part of `IRExecutionUnit::FindSymbol`.

> I believe the usual 'iterate over all language plugins' approach would again 
> work here? We can guess from the mangled symbol what language we deal with 
> (and in the worst case we could encode that in the IRExecutionUnit), so the 
> plugins would just deliver alternative names. 
> `GetAlternativeSymbolNames(ConstString symbol)` or something like that maybe?

I definitely think that the usual "iterate over all language plugins" approach 
would work here if we don't delete this (which it seems like we shouldn't). I 
mostly uploaded this change to see if anybody knew how to trigger this 
codepath. Before refactoring I'd like to write a test for this codepath, though 
I'm still not entirely sure how to do that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108395

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

[Lldb-commits] [PATCH] D108351: [lldb server] Tidy up LLDB server return codes and associated tests

[Raphael Isemann via Phabricator via lldb-commits]

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

LGTM, thanks! I'll merge this on Monday when I have time to watch the bots 
(unless ofc someone else merges this before then).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108351

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

[Lldb-commits] [PATCH] D108351: [lldb server] Tidy up LLDB server return codes and associated tests

[Sebastian Schwartz via Phabricator via lldb-commits]

saschwartz updated this revision to Diff 367815.
saschwartz added a comment.

Stick with integer return codes from lldb-platform


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108351

Files:
  lldb/test/Shell/lldb-server/TestErrorMessages.test
  lldb/test/Shell/lldb-server/TestGdbserverPort.test
  lldb/tools/lldb-server/lldb-platform.cpp
  lldb/tools/lldb-server/lldb-server.cpp

Index: lldb/tools/lldb-server/lldb-server.cpp
===
--- lldb/tools/lldb-server/lldb-server.cpp
+++ lldb/tools/lldb-server/lldb-server.cpp
@@ -11,6 +11,7 @@
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/ManagedStatic.h"
@@ -52,29 +53,30 @@
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);
   llvm::PrettyStackTraceProgram X(argc, argv);
 
-  int option_error = 0;
   const char *progname = argv[0];
   if (argc < 2) {
 display_usage(progname);
-exit(option_error);
+return 1;
   }
 
   switch (argv[1][0]) {
-  case 'g':
+  case 'g': {
 llgs::initialize();
-main_gdbserver(argc, argv);
-llgs::terminate_debugger();
-break;
-  case 'p':
+auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
+return main_gdbserver(argc, argv);
+  }
+  case 'p': {
 llgs::initialize();
-main_platform(argc, argv);
-llgs::terminate_debugger();
-break;
-  case 'v':
+auto deinit = llvm::make_scope_exit([]() { llgs::terminate_debugger(); });
+return main_platform(argc, argv);
+  }
+  case 'v': {
 fprintf(stderr, "%s\n", lldb_private::GetVersion());
-break;
-  default:
+return 0;
+  }
+  default: {
 display_usage(progname);
-exit(option_error);
+return 1;
+  }
   }
 }
Index: lldb/tools/lldb-server/lldb-platform.cpp
===
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -92,7 +92,6 @@
   "log-channel-list] [--port-file port-file-path] --server "
   "--listen port\n",
   progname, subcommand);
-  exit(0);
 }
 
 static Status save_socket_id_to_file(const std::string _id,
@@ -165,7 +164,6 @@
   FileSpec socket_file;
   bool show_usage = false;
   int option_error = 0;
-  int socket_error = -1;
 
   std::string short_options(OptionParser::GetShortOptionString(g_long_options));
 
@@ -249,7 +247,7 @@
   }
 
   if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
-return -1;
+return 1;
 
   // Make a port map for a port range that was specified.
   if (min_gdbserver_port && min_gdbserver_port < max_gdbserver_port) {
@@ -269,7 +267,7 @@
 
   if (show_usage || option_error) {
 display_usage(progname, subcommand);
-exit(option_error);
+return option_error;
   }
 
   // Skip any options we consumed with getopt_long_only.
@@ -288,13 +286,13 @@
   listen_host_port, children_inherit_listen_socket, error));
   if (error.Fail()) {
 fprintf(stderr, "failed to create acceptor: %s", error.AsCString());
-exit(socket_error);
+return 1;
   }
 
   error = acceptor_up->Listen(backlog);
   if (error.Fail()) {
 printf("failed to listen: %s\n", error.AsCString());
-exit(socket_error);
+return 1;
   }
   if (socket_file) {
 error =
@@ -322,7 +320,7 @@
 error = acceptor_up->Accept(children_inherit_accept_socket, conn);
 if (error.Fail()) {
   WithColor::error() << error.AsCString() << '\n';
-  exit(socket_error);
+  return 1;
 }
 printf("Connection established.\n");
 if (g_server) {
Index: lldb/test/Shell/lldb-server/TestGdbserverPort.test
===
--- lldb/test/Shell/lldb-server/TestGdbserverPort.test
+++ lldb/test/Shell/lldb-server/TestGdbserverPort.test
@@ -1,4 +1,4 @@
 # Windows does not build lldb-server.
 # UNSUPPORTED: system-windows
-# RUN: %platformserver --server --listen :1234 --min-gdbserver-port 1234 --max-gdbserver-port 1234 2>&1 | FileCheck %s
+# RUN: not %platformserver --server --listen :1234 --min-gdbserver-port 1234 --max-gdbserver-port 1234 2>&1 | FileCheck %s
 # CHECK: error: --min-gdbserver-port (1234) is not lower than --max-gdbserver-port (1234)
Index: lldb/test/Shell/lldb-server/TestErrorMessages.test
===
--- lldb/test/Shell/lldb-server/TestErrorMessages.test
+++ lldb/test/Shell/lldb-server/TestErrorMessages.test
@@ -1,14 +1,26 @@
-RUN: %lldb-server gdbserver --fd 2>&1 | FileCheck --check-prefixes=FD1,ALL %s
+RUN: not %lldb-server gdbserver --fd 2>&1 | FileCheck --check-prefixes=FD1,GDB_REMOTE_ALL %s
 FD1: error: --fd: missing argument
 
-RUN: %lldb-server gdbserver --fd three 2>&1 | FileCheck 

[Lldb-commits] [PATCH] D108351: [lldb server] Tidy up LLDB server return codes and associated tests

[Sebastian Schwartz via Phabricator via lldb-commits]

saschwartz marked 5 inline comments as done.
saschwartz added inline comments.



Comment at: lldb/tools/lldb-server/lldb-platform.cpp:289
 fprintf(stderr, "failed to create acceptor: %s", error.AsCString());
-exit(socket_error);
+return -1;
   }

teemperor wrote:
> saschwartz wrote:
> > teemperor wrote:
> > > clayborg wrote:
> > > > Should we return error.GetError() if it is non zero? IIRC it will be 
> > > > the actual errno. And best to not return -1, just return 1.
> > > > 
> > > > ```
> > > > uint32_t SBError::GetError() const;
> > > > ```
> > > If we force the caller to convert errno to an exit code, then we could 
> > > also just return the `Status error` itself (and then the caller can just 
> > > return 0/1 depending on success or error)? That seems more clear than 
> > > returning `errno` from a function with main signature (which makes it 
> > > look like it would return an exit code).
> > Sounds fine to me - I went with `-1` because that was the original value 
> > for `socket_error`, but don't think anything should be conditioning on 
> > that. 
> > 
> > I'm pretty ambivalent to the `Status error` vs an error code directly 
> > myself, mainly because I don't know LLVM well enough to know what the 
> > convention might be. Will `error.GetError` always be nonzero if 
> > `error.Fail()` is true?
> As said above, for this to work we need to have the caller still transform 
> the error code into a valid exit code. Status will give us any integer back 
> (errno or something else we made up), but if we return that from `main` then 
> the exit code will be set on UNIX to the lowest 8 bits of that value. So 
> essentially right now we implicitly do `exit_code = error % 256`. That only 
> works if the system agrees to never use a multiple of 256 as an error code 
> and the same goes for LLDB internally. And then there is also all the other 
> weird stuff other operating systems will do here.
> 
> I don't see any other error code in LLVM beside 0/1/-1 so let's just keep 
> this patch simple and return one of those from `main`. I don't think it 
> matters a lot what we return from this artificial main method, but if it's an 
> `int` then it looks like an exit code from the function signature and it 
> should be a reasonable exit code. So if we want to return an actual error 
> code then it should be wrapped in a `Status` to make it clear to the caller 
> that they need to convert it to an exit code.
> 
> > Will error.GetError always be nonzero if error.Fail() is true?
> 
> Yes, `GetError` returns non-zero on failure, but the clearer check is 
> `!error.Success()` (which does the same check under the hood).
Sounds fine. That's a good argument about the implicit `% 256` operation which 
I tend to agree would be an unexpected operation for the caller to have to deal 
with. Sounds fine to stick with the typical semantics of a `main` function and 
just `return 1` for all errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108351

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

[Lldb-commits] [PATCH] D108468: [lldb] [gdb-remote] Fix displaying i387_ext & vec regs with gdbserver

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski, JDevlieghere, jasonmolenda.
mgorny requested review of this revision.

Adjust the encoding and format applied to i387_ext and vec* type
registers from gdbserver to match lldb-server.  Both types are now
displayed as vector of uint8 instead of float and integer formats used
before.  Additionally, this fixes display of STi registers when they do
not carry floating-point data (they are also used to hold MMX vectors).

Add client tests using GDB-style target.xml.


https://reviews.llvm.org/D108468

Files:
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py
@@ -0,0 +1,98 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestGDBServerTargetXML(GDBRemoteTestBase):
+
+@skipIfXmlSupportMissing
+@skipIfRemote
+def test_x86_64_vec_regs(self):
+"""Test rendering of x86_64 vector registers from gdbserver."""
+class MyResponder(MockGDBServerResponder):
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return """
+
+
+  i386:x86-64
+  GNU/Linux
+  
+
+
+
+
+
+
+
+
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+""", False
+else:
+return None, False
+
+def readRegister(self, regnum):
+return ""
+
+def readRegisters(self):
+return (
+"0102030405060708"  # rsp
+"1112131415161718"  # rip
+"0102030405060708090a"  # st0
+"1112131415161718191a" +  # st1
+"2122232425262728292a" * 6 +  # st2..st7
+"8182838485868788898a8b8c8d8e8f90"  # xmm0
+"9192939495969798999a9b9c9d9e9fa0"  # xmm1
+"a1a2a3a4a5a6a7a8a9aaabacadaeafb0"  # xmm2..xmm7
+)
+
+def haltReason(self):
+return "T02thread:1ff0d;threads:1ff0d;thread-pcs:00010001bc00;07:0102030405060708;10:1112131415161718;"
+
+self.server.responder = MyResponder()
+
+target = self.createTarget("basic_eh_frame.yaml")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+  [lldb.eStateStopped])
+
+# rsp and rip should be displayed as uints
+self.match("register read rsp",
+   ["rsp = 0x0807060504030201"])
+self.match("register read rip",
+   ["rip = 0x1817161514131211"])
+
+# both stX and xmmX should be displayed as vectors
+self.match("register read st0",
+   ["st0 = {0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a}"])
+self.match("register read st1",
+   ["st1 = {0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a}"])
+self.match("register read xmm0",
+   ["xmm0 = {0x81 0x82 0x83 0x84 0x85 0x86 0x87 0x88 "
+"0x89 0x8a 0x8b 0x8c 0x8d 0x8e 0x8f 0x90}"])
+self.match("register read xmm1",
+   ["xmm1 = {0x91 0x92 0x93 0x94 0x95 0x96 0x97 0x98 "
+"0x99 0x9a 0x9b 0x9c 0x9d 0x9e 0x9f 0xa0}"])
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4488,9 +4488,13 @@
   } else if (gdb_type == 

[Lldb-commits] [PATCH] D108395: [lldb] Delete IRExecutionUnit::CollectCandidateCPlusPlusNames

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

I'm very supportive of the idea that we delete everything that isn't tested :) 
(and I'm only partly joking here). However removing this should cause a bunch 
of test failures on Linux as it effectively disables D70721 
 (the test added in that patch was just a unit 
test that is preserved with this patch, but the bigger problem describes there 
will break a few tests calling constructors on Linux).

I believe the usual 'iterate over all language plugins' approach would again 
work here? We can guess from the mangled symbol what language we deal with (and 
in the worst case we could encode that in the IRExecutionUnit), so the plugins 
would just deliver alternative names. `GetAlternativeSymbolNames(ConstString 
symbol)` or something like that maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108395

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

[Lldb-commits] [PATCH] D107456: [lldb] Support .debug_rnglists.dwo sections in dwp file

[Kim-Anh Tran via Phabricator via lldb-commits]

kimanh added a comment.

Thanks a lot for the review! Very sorry for my late update, I was out on 
vacation and then had to catch up with mails. Updated the revision now.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:537
+if (const llvm::DWARFUnitIndex::Entry *entry = m_header.GetIndexEntry()) {
+  const auto *contribution = 
entry->getContribution(llvm::DW_SECT_RNGLISTS);
+  if (!contribution) {

jankratochvil wrote:
> I have found this `getContribution` adjustment duplication, are there some 
> reasons it is not unified?  
> https://www.jankratochvil.net/t/D107456-unify.patch
> The issue is the code then handles two different `.debug_rnglists` 
> `DWARFDataExtractor`s with different offsets.
> 
Thanks a lot for the patch, I unified it in the latest update. IIRC (some time 
ago, so I don't recall completely) there was a mismatch in offsets maybe 
because I missed to update `GetRnglistOffset` to use `getRnglistData` too.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:556
+  " (ranges list base: 0x%" PRIx64 "): %s",
+  offset, m_ranges_base, toString(table_or_error.takeError()).c_str());
   }

jankratochvil wrote:
> One such reason can be missing DWP absolute offset for the error report. That 
> could be returned from `GetRnglistData()`.
> 
If I understand your comment correctly, you are suggesting to incorporate the 
contribution offset into the error message of `GetRnglistData`, is that 
correct? However, `GetRnglistData` will only return an error message if the 
contribution offset cannot be read, so the contribution offset cannot be 
specified in the error message (and thus the absolute offset cannot be 
inferred) . Or maybe I'm misunderstanding your comment here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107456

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

[Lldb-commits] [PATCH] D107456: [lldb] Support .debug_rnglists.dwo sections in dwp file

[Kim-Anh Tran via Phabricator via lldb-commits]

kimanh updated this revision to Diff 367758.
kimanh marked 2 inline comments as done.
kimanh added a comment.

- Incorporating Jan's patch
- Adding error message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107456

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwp.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwp.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwp.s
@@ -0,0 +1,187 @@
+## This tests if .debug_rnglists.dwo are correctly read if they are part
+## of a dwp file.
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s --defsym MAIN=0 > %t
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t.dwp
+# RUN: %lldb %t -o "image lookup -v -s lookup_rnglists" -o exit | FileCheck %s
+
+# CHECK-LABEL: image lookup -v -s lookup_rnglists
+# CHECK:  Function: id = {{.*}}, name = "rnglists", range = [0x-0x0003)
+# CHECK:Blocks: id = {{.*}}, range = [0x-0x0003)
+# CHECK-NEXT:   id = {{.*}}, range = [0x0001-0x0002)
+
+.text
+rnglists:
+nop
+.Lblock1_begin:
+lookup_rnglists:
+nop
+.Lblock1_end:
+nop
+.Lrnglists_end:
+
+## The main file.
+.ifdef MAIN
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   0   # DW_CHILDREN_no
+.byte   0x76# DW_AT_dwo_name
+.byte   8   # DW_FORM_string
+.byte   115 # DW_AT_addr_base
+.byte   23  # DW_FORM_sec_offset
+.byte   85  # DW_AT_ranges
+.byte   35  # DW_FORM_rnglistx
+.byte   116 # DW_AT_rnglists_base
+.byte   23  # DW_FORM_sec_offset
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   0   # EOM(3)
+
+.section.debug_info,"",@progbits
+.Lcu_begin0:
+.long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+.short  5   # DWARF version number
+.byte   4   # DWARF Unit Type
+.byte   8   # Address Size (in bytes)
+.long   .debug_abbrev   # Offset Into Abbrev. Section
+.quad   1026699901672188186 # DWO id
+.byte   1   # Abbrev [1] DW_TAG_compile_unit
+.asciz  "debug_rnglists-dwp.s.tmp.dwo"  # DW_AT_dwo_name
+.long   .Laddr_table_base0  # DW_AT_addr_base
+.byte   0   # DW_AT_ranges
+.long   .Lskel_rnglists_table_base # DW_AT_rnglists_base
+.Ldebug_info_end0:
+
+.section.debug_addr,"",@progbits
+.long   .Ldebug_addr_end0-.Ldebug_addr_start0 # Length of contribution
+.Ldebug_addr_start0:
+.short  5   # DWARF version number
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.Laddr_table_base0:
+.quad   rnglists
+.quad   .Lblock1_begin
+.Ldebug_addr_end0:
+
+.section.debug_rnglists,"",@progbits
+.long   .Lskel_rnglist_table_end-.Lskel_rnglist_table_start # Length
+.Lskel_rnglist_table_start:
+.short  5   # Version
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.long   1   # Offset entry count
+.Lskel_rnglists_table_base:
+.long   .Lskel_ranges0-.Lskel_rnglists_table_base
+.Lskel_ranges0:
+.byte   7   # DW_RLE_start_length
+.quad   rnglists
+.uleb128   .Lrnglists_end-rnglists
+.byte   0   # DW_RLE_end_of_list
+.Lskel_rnglist_table_end:
+ .else
+ ## DWP file starts here.
+.section.debug_abbrev.dwo,"e",@progbits
+.LAbbrevBegin:
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   37  # DW_AT_producer
+.byte   8   # DW_FORM_string
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   2   # Abbreviation Code
+.byte   46  # DW_TAG_subprogram
+.byte   1   # 

[Lldb-commits] [PATCH] D108351: [lldb server] Tidy up LLDB server return codes and associated tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/tools/lldb-server/lldb-platform.cpp:289
 fprintf(stderr, "failed to create acceptor: %s", error.AsCString());
-exit(socket_error);
+return -1;
   }

saschwartz wrote:
> teemperor wrote:
> > clayborg wrote:
> > > Should we return error.GetError() if it is non zero? IIRC it will be the 
> > > actual errno. And best to not return -1, just return 1.
> > > 
> > > ```
> > > uint32_t SBError::GetError() const;
> > > ```
> > If we force the caller to convert errno to an exit code, then we could also 
> > just return the `Status error` itself (and then the caller can just return 
> > 0/1 depending on success or error)? That seems more clear than returning 
> > `errno` from a function with main signature (which makes it look like it 
> > would return an exit code).
> Sounds fine to me - I went with `-1` because that was the original value for 
> `socket_error`, but don't think anything should be conditioning on that. 
> 
> I'm pretty ambivalent to the `Status error` vs an error code directly myself, 
> mainly because I don't know LLVM well enough to know what the convention 
> might be. Will `error.GetError` always be nonzero if `error.Fail()` is true?
As said above, for this to work we need to have the caller still transform the 
error code into a valid exit code. Status will give us any integer back (errno 
or something else we made up), but if we return that from `main` then the exit 
code will be set on UNIX to the lowest 8 bits of that value. So essentially 
right now we implicitly do `exit_code = error % 256`. That only works if the 
system agrees to never use a multiple of 256 as an error code and the same goes 
for LLDB internally. And then there is also all the other weird stuff other 
operating systems will do here.

I don't see any other error code in LLVM beside 0/1/-1 so let's just keep this 
patch simple and return one of those from `main`. I don't think it matters a 
lot what we return from this artificial main method, but if it's an `int` then 
it looks like an exit code from the function signature and it should be a 
reasonable exit code. So if we want to return an actual error code then it 
should be wrapped in a `Status` to make it clear to the caller that they need 
to convert it to an exit code.

> Will error.GetError always be nonzero if error.Fail() is true?

Yes, `GetError` returns non-zero on failure, but the clearer check is 
`!error.Success()` (which does the same check under the hood).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108351

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