[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
labath added a comment. I am confused by differing scopes of various objects that are interacting here. It seems you are implementing capture/replay as something that can be flicked on/off at any point during a debug session (`Debugger` lifetime). However, you are modifying global state (the filesystem singleton, at least), which is shared by all debug sessions. If multiple debug sessions try to enable capture/replay (or even access the filesystem concurrently, as none of this is synchronized in any way), things will break horribly. This seems like a bad starting point in implementing a feature, as I don't see any way to fix incrementally in the future. Maybe you meant that by `Initialization of the FS needs to happen in the driver.`? In any case, for me the initialization part is the interesting/hard part of this problem. The details of how to collect the files are trivial compared to that and possibly depend on the result of the decisions made there. Repository: rLLDB LLDB https://reviews.llvm.org/D54617 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider
JDevlieghere added a comment. In https://reviews.llvm.org/D54617#1302366, @labath wrote: > I am confused by differing scopes of various objects that are interacting > here. It seems you are implementing capture/replay as something that can be > flicked on/off at any point during a debug session (`Debugger` lifetime). > However, you are modifying global state (the filesystem singleton, at least), > which is shared by all debug sessions. If multiple debug sessions try to > enable capture/replay (or even access the filesystem concurrently, as none of > this is synchronized in any way), things will break horribly. This seems like > a bad starting point in implementing a feature, as I don't see any way to fix > incrementally in the future. Maybe I should've added a comment (I didn't want to litter the code with design goals) but the goal of the commands is not really to enable/disable the reproducer feature. I'm currently abusing the commands to do that, but the long term goal is that you can enable/disable part of the reproducer. For example, you could say only capture GDB packets or capture only files. It makes testing a lot easier if you can isolate a single source of information. Since there's currently only GDB packets I didn't split it up (yet). Also I piggy-backed off this to enable/disable the feature as a whole because I needed to go through the debugger. See my reply below on why and how that'll be fixed by the next patch :-) > Maybe you meant that by `Initialization of the FS needs to happen in the > driver.`? In any case, for me the initialization part is the interesting/hard > part of this problem. The details of how to collect the files are trivial > compared to that and possibly depend on the result of the decisions made > there. Yup, as per your suggestion in another review. I have an (unfinished) patch that splits off parsing of the command line arguments in the driver so we know the reproducer path before we initialize anything. That's pretty much the whole reason that we currently go through the debugger right now. Repository: rLLDB LLDB https://reviews.llvm.org/D54617 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r347174 - Revert "Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD"
Author: zturner
Date: Sun Nov 18 12:48:25 2018
New Revision: 347174
URL: http://llvm.org/viewvc/llvm-project?rev=347174&view=rev
Log:
Revert "Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD"
This breaks many tests on Windows, which now all fail with an error such
as "Unable to read memory at address ".
Removed:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/windows_dyld/
Modified:
lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
Modified: lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules?rev=347174&r1=347173&r2=347174&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules Sun Nov 18
12:48:25 2018
@@ -535,7 +535,7 @@ EXE = $(DYLIB_FILENAME)
endif
else
$(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
- "$(LD)" $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+ $(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
ifneq "$(CODESIGN)" ""
$(CODESIGN) -s - "$(EXE)"
endif
@@ -592,7 +592,7 @@ ifneq "$(DS)" ""
endif
endif
else
- "$(LD)" $(DYLIB_OBJECTS) $(LDFLAGS) -shared -o "$(DYLIB_FILENAME)"
+ $(LD) $(DYLIB_OBJECTS) $(LDFLAGS) -shared -o "$(DYLIB_FILENAME)"
ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
$(OBJCOPY) --only-keep-debug "$(DYLIB_FILENAME)"
"$(DYLIB_FILENAME).debug"
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DYLIB_FILENAME).debug"
"$(DYLIB_FILENAME)" "$(DYLIB_FILENAME)"
Modified:
lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp?rev=347174&r1=347173&r2=347174&view=diff
==
---
lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
(original)
+++
lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
Sun Nov 18 12:48:25 2018
@@ -10,14 +10,12 @@
#include "DynamicLoaderWindowsDYLD.h"
-#include "lldb/Core/Module.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Target/ExecutionContext.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/ThreadPlanStepInstruction.h"
-#include "lldb/Utility/Log.h"
#include "llvm/ADT/Triple.h"
@@ -62,49 +60,9 @@ DynamicLoader *DynamicLoaderWindowsDYLD:
return nullptr;
}
-void DynamicLoaderWindowsDYLD::DidAttach() {
- Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
- if (log)
-log->Printf("DynamicLoaderWindowsDYLD::%s()", __FUNCTION__);
+void DynamicLoaderWindowsDYLD::DidAttach() {}
- DidLaunch();
-
- m_process->LoadModules();
-}
-
-void DynamicLoaderWindowsDYLD::DidLaunch() {
- Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
- if (log)
-log->Printf("DynamicLoaderWindowsDYLD::%s()", __FUNCTION__);
-
- ModuleSP executable = GetTargetExecutable();
-
- if (!executable.get())
-return;
-
- // Try to fetch the load address of the file from the process, since there
- // could be randomization of the load address.
-
- // It might happen that the remote has a different dir for the file, so we
- // only send the basename of the executable in the query. I think this is
safe
- // because I doubt that two executables with the same basenames are loaded in
- // memory...
- FileSpec file_spec(
- executable->GetPlatformFileSpec().GetFilename().GetCString());
- bool is_loaded;
- addr_t base_addr = 0;
- lldb::addr_t load_addr;
- Status error = m_process->GetFileLoadAddress(file_spec, is_loaded,
load_addr);
- if (error.Success() && is_loaded) {
-base_addr = load_addr;
- }
-
- UpdateLoadedSections(executable, LLDB_INVALID_ADDRESS, base_addr, false);
-
- ModuleList module_list;
- module_list.Append(executable);
- m_process->GetTarget().ModulesDidLoad(module_list);
-}
+void DynamicLoaderWindowsDYLD::DidLaunch() {}
Status DynamicLoaderWindowsDYLD::CanLoadImage() { return Status(); }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54544: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD
zturner added a subscriber: lanza. zturner added a comment. I had to revert this because it breaks many tests on Windows (found by bisecting). It was reverted in r347174. You can reproduce one of the failures by building with this patch applied, then doing python bin/llvm-lit.py -sv ~/src/lldb/lit/SymbolFile/PDB. 2 of the tests should fail and you see an error about unable to read memory location. Repository: rLLDB LLDB https://reviews.llvm.org/D54544 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D54544: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD
I had to revert this because it breaks many tests on Windows (found by
bisecting). It was reverted in r347174.
You can reproduce one of the failures by building with this patch applied,
then doing python bin/llvm-lit.py -sv ~/src/lldb/lit/SymbolFile/PDB.
2 of the tests should fail and you see an error about unable to read memory
location.
On Thu, Nov 15, 2018 at 1:38 PM Hui Huang via Phabricator <
revi...@reviews.llvm.org> wrote:
> Hui added inline comments.
>
>
>
> Comment at:
> source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:75
> +
> +void DynamicLoaderWindowsDYLD::DidLaunch() {
> + Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
>
> I think DynamicLoaderWindowsDYLD::DidAttach & DidLaunch are intended for
> remote debugging. As the similar functionalities have been done in
> ProcessWindows::DidLaunch & DidAttach, the test in here is actually testing
> those native ones and they happen to be correct.
>
>
>
> Comment at:
> source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:102
> +
> + UpdateLoadedSections(executable, LLDB_INVALID_ADDRESS, base_addr,
> false);
>
>
> A remote debugging shows if the 'qFileLoadAddress' remote packet is not
> implemented or incorrectly handled, GetFileLoadAddress by the remote
> process will return error resulting base_addr stay '0. Since you are
> calling with the last argument 'false' to indicate it is not an offset,
> that will be an issue. Better to move it to line 99 and check if the
> load_addr is LLDB_INVALID_ADDRESS.
>
> Moreover, if the load_addr is changed, make sure all the breakpoint
> address is recalculated. This feature could be a little bit complicated.
>
>
> Repository:
> rLLDB LLDB
>
> https://reviews.llvm.org/D54544
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r347109 - Rewrite stop-hook tests as a couple of FileCheck tests
Different shells have different quoting rules, so unfortunately something
like this:
# RUN: %lldb -b -s %s -O 'target create %t' -O 'target stop-hook add -n b
-o "expr ptr"'
is non-portable. For example, on Windows all single quotes are converted
to double quotes before running, so this is effectively:
lldb.EXE
-S lit-lldb-init
-b
-s stop-hook.test
-O "target create stop-hook.test.tmp"
-O "target stop-hook add -n b -o expr" // expr gets concatenated
to the previous quoted string since there is no space separating expr and
the closing quote from the previous arg.
ptr
If we're going to be writing more of these kinds of tests (and I think we
should!), I think we need to agree that we should not be using -O, instead
we should source the commands from a file.
I'll prepare a patch that converts these two tests to do just that and add
you as a reviewer.
On Fri, Nov 16, 2018 at 3:09 PM Frederic Riss via lldb-commits <
lldb-commits@lists.llvm.org> wrote:
> Author: friss
> Date: Fri Nov 16 15:07:28 2018
> New Revision: 347109
>
> URL: http://llvm.org/viewvc/llvm-project?rev=347109&view=rev
> Log:
> Rewrite stop-hook tests as a couple of FileCheck tests
>
> Those tests were using pexpect and being flaky on some of ours bots.
> This patch reimplmeents the tests usinf FileCheck, and it also
> extends the test coverage to a few more stop-hook options.
>
> Added:
> lldb/trunk/lit/ExecControl/
> lldb/trunk/lit/ExecControl/StopHook/
> lldb/trunk/lit/ExecControl/StopHook/Inputs/
> lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook-threads.cpp
> - copied, changed from r347104,
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/main.cpp
> lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook.c
> - copied, changed from r347104,
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/main.cpp
> lldb/trunk/lit/ExecControl/StopHook/stop-hook-threads.test
> lldb/trunk/lit/ExecControl/StopHook/stop-hook.test
> Removed:
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/Makefile
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/TestStopHookCmd.py
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/TestStopHookMechanism.py
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/main.cpp
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/Makefile
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/TestStopHookMultipleThreads.py
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/main.cpp
>
> Copied: lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook-threads.cpp
> (from r347104,
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/main.cpp)
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook-threads.cpp?p2=lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook-threads.cpp&p1=lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/main.cpp&r1=347104&r2=347109&rev=347109&view=diff
>
> ==
> ---
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/stop-hook/multiple_threads/main.cpp
> (original)
> +++ lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook-threads.cpp Fri
> Nov 16 15:07:28 2018
> @@ -14,7 +14,7 @@
> #include
>
> std::default_random_engine g_random_engine{std::random_device{}()};
> -std::uniform_int_distribution<> g_distribution{0, 300};
> +std::uniform_int_distribution<> g_distribution{0, 3000};
>
> uint32_t g_val = 0;
>
> @@ -42,14 +42,14 @@ thread_func (uint32_t thread_index)
>
> uint32_t count = 0;
> uint32_t val;
> -while (count++ < 15)
> +while (count++ < 4)
> {
> -// random micro second sleep from zero to 3 seconds
> +// random micro second sleep from zero to .3 seconds
> int usec = g_distribution(g_random_engine);
> printf ("%s (thread = %u) doing a usleep (%d)...\n",
> __FUNCTION__, thread_index, usec);
> -std::this_thread::sleep_for(std::chrono::microseconds{usec});
> +std::this_thread::sleep_for(std::chrono::microseconds{usec}); //
> Set break point at this line
>
> -if (count < 7)
> +if (count < 2)
> val = access_pool ();
> else
> val = access_pool (true);
> @@ -64,7 +64,6 @@ int main (int argc, char const *argv[])
> {
> std::thread threads[3];
>
> -printf ("Before turning all three threads loose...\n"); // Set break
> point at this line, and add a stop-hook.
> // Create 3 threads
> for (auto &thread : threads)
> thread = std::thread{thread_func, std::distance(threads,
> &thread)};
>
> Copied: lldb/trunk/lit/ExecControl/StopHook/Inputs/stop-hook.c (from
> r347104,
[Lldb-commits] [PATCH] D54680: Don't use lldb -O in lit tests
zturner created this revision. zturner added a reviewer: friss. Because of different shell quoting rules, and the fact that LLDB commands often contain spaces, -O is not portable for writing command lines. Instead, we should use explicit lldbinit files. https://reviews.llvm.org/D54680 Files: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-1.lldbinit lldb/lit/ExecControl/StopHook/Inputs/stop-hook-2.lldbinit lldb/lit/ExecControl/StopHook/Inputs/stop-hook-3.lldbinit lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-1.lldbinit lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-2.lldbinit lldb/lit/ExecControl/StopHook/stop-hook-threads.test lldb/lit/ExecControl/StopHook/stop-hook.test Index: lldb/lit/ExecControl/StopHook/stop-hook.test === --- lldb/lit/ExecControl/StopHook/stop-hook.test +++ lldb/lit/ExecControl/StopHook/stop-hook.test @@ -1,11 +1,11 @@ # RUN: %cc %p/Inputs/stop-hook.c -g -o %t # Test setting stop-hook per-function -# RUN: %lldb -b -s %s -O 'target create %t' -O 'target stop-hook add -n b -o "expr ptr"' \ +# RUN: %lldb -b -s %p/Inputs/stop-hook-1.lldbinit -s %s -f %t \ # RUN: | FileCheck --check-prefix=CHECK --check-prefix=CHECK-FUNC %s # Test setting stop-hook per-line range -# RUN: %lldb -b -s %s -O 'target create %t' -O 'target stop-hook add -f stop-hook.c -l 30 -e 34 -o "expr ptr"' | FileCheck %s +# RUN: %lldb -b -s %p/Inputs/stop-hook-2.lldbinit -s %s -f %t | FileCheck %s # Test setting stop-hook with multi-line expression -# RUN: %lldb -b -s %s -O 'target create %t' -O 'target stop-hook add -f stop-hook.c -l 30 -e 34' -O 'expr ptr' -O DONE | FileCheck %s +# RUN: %lldb -b -s %p/Inputs/stop-hook-3.lldbinit -s %s -f %t | FileCheck %s break set -f stop-hook.c -p "// Set breakpoint here to test target stop-hook" break set -f stop-hook.c -p "// Another breakpoint which is outside of the stop-hook range" Index: lldb/lit/ExecControl/StopHook/stop-hook-threads.test === --- lldb/lit/ExecControl/StopHook/stop-hook-threads.test +++ lldb/lit/ExecControl/StopHook/stop-hook-threads.test @@ -1,19 +1,7 @@ # RUN: %cxx %p/Inputs/stop-hook-threads.cpp -g -o %t -# RUN: %lldb -b -s %s -O 'target create %t' \ -# RUN: -O 'break set -f stop-hook-threads.cpp -p "Break here to test that the stop-hook"' \ -# RUN: -O run \ -# RUN: -O 'target stop-hook add' \ -# RUN:-O "frame variable --show-globals g_val" \ -# RUN:-O "thread list" \ -# RUN:-O continue \ -# RUN:-O DONE \ +# RUN: %lldb -b -s %p/Inputs/stop-hook-threads-1.lldbinit -s %s -f %t \ # RUN: | FileCheck --check-prefix=CHECK --check-prefix=CHECK-NO-FILTER %s -# RUN: %lldb -b -s %s \ -# RUN: -O 'target create %t' \ -# RUN: -O 'break set -f stop-hook-threads.cpp -p "Break here to test that the stop-hook"' \ -# RUN: -O run \ -# RUN: -O 'target stop-hook add -x 2 -o "frame variable thread_index"' \ -# RUN: -O 'target stop-hook add -o continue' \ +# RUN: %lldb -b -s %p/Inputs/stop-hook-threads-2.lldbinit -s %s -f %t \ # RUN: | FileCheck --check-prefix=CHECK --check-prefix=CHECK-FILTER %s thread list Index: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-2.lldbinit === --- /dev/null +++ lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-2.lldbinit @@ -0,0 +1,4 @@ +break set -f stop-hook-threads.cpp -p "Break here to test that the stop-hook" +run +target stop-hook add -x 2 -o "frame variable thread_index" +target stop-hook add -o continue Index: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-1.lldbinit === --- /dev/null +++ lldb/lit/ExecControl/StopHook/Inputs/stop-hook-threads-1.lldbinit @@ -0,0 +1,7 @@ +break set -f stop-hook-threads.cpp -p "Break here to test that the stop-hook" +run +target stop-hook add +frame variable --show-globals g_val +thread list +continue +DONE Index: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-3.lldbinit === --- /dev/null +++ lldb/lit/ExecControl/StopHook/Inputs/stop-hook-3.lldbinit @@ -0,0 +1,3 @@ +target stop-hook add -f stop-hook.c -l 30 -e 34 +expr ptr +DONE \ No newline at end of file Index: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-2.lldbinit === --- /dev/null +++ lldb/lit/ExecControl/StopHook/Inputs/stop-hook-2.lldbinit @@ -0,0 +1 @@ +target stop-hook add -f stop-hook.c -l 30 -e 34 -o "expr ptr" \ No newline at end of file Index: lldb/lit/ExecControl/StopHook/Inputs/stop-hook-1.lldbinit === --- /dev/null +++ lldb/lit/ExecControl/StopHook/Inputs/stop-hook-1.lldbinit @@ -0,0 +1 @@ +target stop-hook add -n b -o "expr ptr" I
[Lldb-commits] [PATCH] D54680: Don't use lldb -O in lit tests
davide requested changes to this revision. davide added a comment. This revision now requires changes to proceed. Is there a way of fixing this that doesn't require scattering the test between two files? https://reviews.llvm.org/D54680 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54680: Don't use lldb -O in lit tests
zturner added a comment. In https://reviews.llvm.org/D54680#1302408, @davide wrote: > Is there a way of fixing this that doesn't require scattering the test > between two files? Unless we create a utility that extracts lines based on prefixes and outputs them to a temporary file, I don't have any great ideas. FWIW I actually find it easier to read this way, the interaction between the various command line options makes it pretty confusing to follow the order in which the various commands are executed. combining -O, -o, -S, and -s makes it quite hard to figure out what commands are actually being executed and in what order. Another advantage to having the commands be in their own file is that it's easy to reproduce the failure by just running `lldb -s script.lldbinit`. I'd even go so far as to say that maybe the .test file should be the source code + check lines, and each of the 3 test cases should be a separate script file with all the commands together in one file. It could use the `command source` command to avoid duplication. So, for example, `stop-hook-1.lldbinit` could be something like: target stop-hook add -f stop-hook.c -l 30 -e 34 -o "expr ptr" command source stop-hook-common.lldbinit https://reviews.llvm.org/D54680 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54680: Don't use lldb -O in lit tests
friss added a comment. I do agree it's slightly easier to read this way. I was conflicted while writing the tests between readability and conciseness. I think this is a good compromise. I do prefer to have the check lines with the commands rather than with the source code though. The output is the result of the commands and sometimes it's not as clear what it refers to in the source. https://reviews.llvm.org/D54680 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, labath, zturner, jingham.
JDevlieghere added a project: LLDB.
In order to deal consistently with global state in LLDB, the reproducer feature
affects LLDB's initialization. For example, when replaying, the FileSystem
singleton is initialized with a virtual file system.
This is a problem for the driver because it initialized the debugger before
parsing command line options. The reason is that the driver, among other
things, checks whether files exists (e.g. core files, target, files to be
sourced). It also relies on the debugger to parse things like the (scripting)
language, the architecture, etc.
In an initial attempt I tried to populate the OptionData before the debugger is
initialized. This proved to be complicated, because of the sanity checks that
are performed by calling into the debugger of the filesystem. Although it would
be possible to perform these checks after parsing, it would cause errors to no
longer appear in the same order as specified by the user, but in an arbitrary
order specified by the driver implementation. Although I like the idea
conceptually I don't believe this is an acceptable regression.
Implemented in this patch is a new `ArgParser` class that extracts the existing
argument parsing logic. Basically it calls `getopt_long_only` repeatedly and
populates a list with the short option and its value. Because the `ArgParser`
is //dumb// it can do all its work before the debugger is initialized.
Afterwards the driver iterates over the options from the argparser (instead of
calling `getopt_long_only` every time) and do whatever is needed.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D54682
Files:
tools/driver/Driver.cpp
tools/driver/Driver.h
Index: tools/driver/Driver.h
===
--- tools/driver/Driver.h
+++ tools/driver/Driver.h
@@ -16,11 +16,47 @@
#include
#include
+#include
+#include
+
#include "lldb/API/SBBroadcaster.h"
#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBError.h"
+class ArgParser {
+public:
+ ArgParser(int argc, const char **);
+
+ void Parse();
+
+ struct ParsedOption {
+ParsedOption(char short_option, std::string option_argument)
+: short_option(short_option), option_argument(option_argument) {}
+char short_option;
+const std::string option_argument;
+ };
+
+ const std::vector &GetParsedOptions() const {
+return m_parsed_options;
+ }
+
+ const std::vector &GetRemainingArguments() const {
+return m_remaining_arguments;
+ }
+
+ bool HasUnknownOrAmbiguousOption() { return m_unknown_or_ambiguous_option; }
+
+private:
+ int m_argc;
+ const char **m_argv;
+ std::vector m_option_table;
+
+ std::vector m_parsed_options;
+ std::vector m_remaining_arguments;
+ bool m_unknown_or_ambiguous_option = false;
+};
+
class Driver : public lldb::SBBroadcaster {
public:
typedef enum CommandPlacement {
@@ -38,8 +74,7 @@
/// @return The exit code that the process should return.
int MainLoop();
- lldb::SBError ParseArgs(int argc, const char *argv[], FILE *out_fh,
- bool &do_exit);
+ lldb::SBError ProcessArgs(ArgParser &parser, FILE *out_fh, bool &do_exit);
const char *GetFilename() const;
Index: tools/driver/Driver.cpp
===
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -15,8 +15,6 @@
#include
#include
#include
-#include
-#include
#include
// Includes for pipe()
@@ -171,10 +169,128 @@
{LLDB_OPT_SET_7, true, "repl-language", 'R', required_argument, 0,
eArgTypeNone, "Chooses the language for the REPL."}};
-static constexpr auto g_num_options = sizeof(g_options)/sizeof(OptionDefinition);
+static constexpr auto g_num_options =
+sizeof(g_options) / sizeof(OptionDefinition);
static const uint32_t last_option_set_with_args = 2;
+static void BuildGetOptTable(std::vector &getopt_table) {
+ getopt_table.resize(g_num_options + 1);
+
+ std::bitset<256> option_seen;
+ uint32_t j = 0;
+ for (const auto &opt : g_options) {
+char short_opt = opt.short_option;
+
+if (option_seen.test(short_opt) == false) {
+ getopt_table[j].name = opt.long_option;
+ getopt_table[j].has_arg = opt.option_has_arg;
+ getopt_table[j].flag = NULL;
+ getopt_table[j].val = opt.short_option;
+ option_seen.set(short_opt);
+ ++j;
+}
+ }
+
+ getopt_table[j].name = NULL;
+ getopt_table[j].has_arg = 0;
+ getopt_table[j].flag = NULL;
+ getopt_table[j].val = 0;
+}
+
+ArgParser::ArgParser(int argc, const char **argv)
+: m_argc(argc), m_argv(argv) {}
+
+void ArgParser::Parse() {
+ // Build option table.
+ BuildGetOptTable(m_option_table);
+
+ assert(!m_option_table.empty());
+
+ // Build the option_string argument for call to getopt_long_only.
+ std::string option_strin
Re: [Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
I’ve often thought we should convert LLDB’s command line parsing code over to use either cl::opt or lib/Option. This would also solve the problem you describe here at the same time. Do you think it’s worth trying to do this? On Sun, Nov 18, 2018 at 7:17 PM Jonas Devlieghere via Phabricator < revi...@reviews.llvm.org> wrote: > JDevlieghere created this revision. > JDevlieghere added reviewers: clayborg, labath, zturner, jingham. > JDevlieghere added a project: LLDB. > > In order to deal consistently with global state in LLDB, the reproducer > feature affects LLDB's initialization. For example, when replaying, the > FileSystem singleton is initialized with a virtual file system. > > This is a problem for the driver because it initialized the debugger > before parsing command line options. The reason is that the driver, among > other things, checks whether files exists (e.g. core files, target, files > to be sourced). It also relies on the debugger to parse things like the > (scripting) language, the architecture, etc. > > In an initial attempt I tried to populate the OptionData before the > debugger is initialized. This proved to be complicated, because of the > sanity checks that are performed by calling into the debugger of the > filesystem. Although it would be possible to perform these checks after > parsing, it would cause errors to no longer appear in the same order as > specified by the user, but in an arbitrary order specified by the driver > implementation. Although I like the idea conceptually I don't believe this > is an acceptable regression. > > Implemented in this patch is a new `ArgParser` class that extracts the > existing argument parsing logic. Basically it calls `getopt_long_only` > repeatedly and populates a list with the short option and its value. > Because the `ArgParser` is //dumb// it can do all its work before the > debugger is initialized. Afterwards the driver iterates over the options > from the argparser (instead of calling `getopt_long_only` every time) and > do whatever is needed. > > > Repository: > rLLDB LLDB > > https://reviews.llvm.org/D54682 > > Files: > tools/driver/Driver.cpp > tools/driver/Driver.h > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
zturner added a subscriber: JDevlieghere. zturner added a comment. I’ve often thought we should convert LLDB’s command line parsing code over to use either cl::opt or lib/Option. This would also solve the problem you describe here at the same time. Do you think it’s worth trying to do this? Repository: rLLDB LLDB https://reviews.llvm.org/D54682 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
JDevlieghere added a comment. In https://reviews.llvm.org/D54682#1302436, @zturner wrote: > I’ve often thought we should convert LLDB’s command line parsing code over > to use either cl::opt or lib/Option. This would also solve the problem you > describe here at the same time. > > Do you think it’s worth trying to do this? I believe both would have the issue that options are not processed in the order specified. I guess it depends on whether people care about this? Happy to give that a shot if the consensus is "nobody cares" :-) Repository: rLLDB LLDB https://reviews.llvm.org/D54682 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54680: Don't use lldb -O in lit tests
davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. I don't have any other great ideas either and I'd say the proposed alternative is not worth the complexity. https://reviews.llvm.org/D54680 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
Lib option definitely does, as its entire purpose is to be powerful and flexible enough to mimic arbitrary command line tools. cl::opt is a little less powerful but it still preserves order among multiple options with the same flag, just not multiple options with different flags. I don’t have a strong opinion that this particular change should be gated on that, it’s just nice when we can reuse llvm logic. Sometimes it fixes latent bugs, sometimes it enables new functionality that wasn’t possible before, etc. up to you if you wanna give it a shot On Sun, Nov 18, 2018 at 7:39 PM Jonas Devlieghere via Phabricator < revi...@reviews.llvm.org> wrote: > JDevlieghere added a comment. > > In https://reviews.llvm.org/D54682#1302436, @zturner wrote: > > > I’ve often thought we should convert LLDB’s command line parsing code > over > > to use either cl::opt or lib/Option. This would also solve the problem > you > > describe here at the same time. > > > > Do you think it’s worth trying to do this? > > > I believe both would have the issue that options are not processed in the > order specified. I guess it depends on whether people care about this? > Happy to give that a shot if the consensus is "nobody cares" :-) > > > Repository: > rLLDB LLDB > > https://reviews.llvm.org/D54682 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.
zturner added a comment. Lib option definitely does, as its entire purpose is to be powerful and flexible enough to mimic arbitrary command line tools. cl::opt is a little less powerful but it still preserves order among multiple options with the same flag, just not multiple options with different flags. I don’t have a strong opinion that this particular change should be gated on that, it’s just nice when we can reuse llvm logic. Sometimes it fixes latent bugs, sometimes it enables new functionality that wasn’t possible before, etc. up to you if you wanna give it a shot Repository: rLLDB LLDB https://reviews.llvm.org/D54682 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
