zturner added a comment.
In https://reviews.llvm.org/D46005#1077013, @zturner wrote:
> In https://reviews.llvm.org/D46005#1077000, @labath wrote:
>
> > In https://reviews.llvm.org/D46005#1076978, @zturner wrote:
> >
> > > Does it print just the names of the .py files, or does it print the
> > >
zturner added a comment.
Actually, for the first case, I'm thinking we can extend lit to support
somethign like per-directory set up. Like if there is a file present called
`lit.init.cfg`, then this file contains some code that is run once per
directory before any of the tests. Then we could
zturner added a comment.
In https://reviews.llvm.org/D46005#1077029, @labath wrote:
> In https://reviews.llvm.org/D46005#1077013, @zturner wrote:
>
> > I thought the intention was going to be parallelize at file-granularity
> > rather than method granularity, since the whole point of grouping te
zturner added a subscriber: labath.
zturner added a comment.
Well let’s see what Davide and Adrian think. I’m more of an outsider these
days so consider my perspective an llvm-centric one, which would sometimes
(but not always) be the best for lldb
https://reviews.llvm.org/D46005
zturner added inline comments.
Comment at: lit/CMakeLists.txt:32
+if(TARGET lld)
+ list(APPEND LLDB_TEST_DEPS lld)
+ set(LLDB_HAVE_LLD 1)
Note that this depends on lld having had its own `CMakeLists.txt` file
processed before LLDB's. I think this happens by a
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298099: Fix some signed/unsigned comparison warnings.
(authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D30926?vs=91670&id=92156#toc
Repository:
rL LLVM
https://reviews.llvm.o
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298100: CMake requires normalized paths when appending.
(authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D30927?vs=91676&id=92157#toc
Repository:
rL LLVM
https://reviews.llvm
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
In the places where you want to read from an `ifstream` and write to a socket,
you might consider using `llvm::sys::fs::copy_file`, declared in
`Support/FileSystem.h`. Currently it takes tw
zturner created this revision.
Use the LLVM function instead.
There are two subtle behavioral changes here which I want to make clear so
someone can determine whether this matters on their platform.
1. Previously all LLDB callers were passing `eFilePermissionsDirectoryDefault`
which is equal t
zturner abandoned this revision.
zturner added a comment.
Messed up reviewer / subscriber.
https://reviews.llvm.org/D31088
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner created this revision.
Use the LLVM functions instead.
https://reviews.llvm.org/D31088
Files:
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/posix/FileSystem.cpp
lldb/source/Host/windows/FileSystem.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerComm
zturner created this revision.
Use the LLVM functions instead
https://reviews.llvm.org/D31089
Files:
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/posix/FileSystem.cpp
lldb/source/Host/windows/FileSystem.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommo
zturner added a comment.
That one is calling a file static function `MakeDirectory`, not
`FileSystem::MakeDirectory`, and the implementation of that function already
calls `llvm::sys::fs::create_directories()` to create the whole tree, so it
should be fine.
https://reviews.llvm.org/D31086
zturner added inline comments.
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:813
+ auto perms =
+ static_cast(packet.GetHexMaxU32(false,
UINT32_MAX));
if (packet.GetChar() == ',') {
labath wrote:
> This doesn
zturner added inline comments.
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:813
+ auto perms =
+ static_cast(packet.GetHexMaxU32(false,
UINT32_MAX));
if (packet.GetChar() == ',') {
zturner wrote:
> labath wr
zturner created this revision.
https://reviews.llvm.org/D31108
Files:
lldb/source/Host/common/FileSystem.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
lldb/source/Target/Platform.cpp
Index: lldb/so
zturner created this revision.
Herald added a subscriber: emaste.
https://reviews.llvm.org/D3
Files:
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/common/File.cpp
lldb/source/Host/common/Host.cpp
lldb/source/Host/macosx/Host.mm
lldb/source/Host/posix/DomainSocket.cpp
lldb/s
zturner updated this revision to Diff 92236.
zturner added a comment.
Forgot to remove `Stat` declaration from header file.
https://reviews.llvm.org/D3
Files:
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/common/File.cpp
lldb/source/Host/common/Host.cpp
lldb/source/Host/macos
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298203: Remove FileSystem::MakeDirectory. (authored by
zturner).
Changed prior to commit:
https://reviews.llvm.org/D31086?vs=92162&id=92259#toc
Repository:
rL LLVM
https://reviews.llvm.org/D31086
F
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298205: Remove FileSystem::Get/SetFilePermissions (authored
by zturner).
Changed prior to commit:
https://reviews.llvm.org/D31089?vs=92167&id=92260#toc
Repository:
rL LLVM
https://reviews.llvm.org/D
zturner created this revision.
Herald added subscribers: mgorny, srhines.
This is the last step before I plan to move `FileSpec` to `Utility`, which
should completely eliminate between 3 and 5 dependencies from other targets to
`Host`.
https://reviews.llvm.org/D31129
Files:
lldb/include/lld
zturner updated this revision to Diff 92287.
zturner added a comment.
Turns out `ResolveUsername` was only being called from one place outside of
`FileSpec` and `ResolvePartialUsername` was dead code (since callers had
already been updated to use `TildeExpressionResolver`. So I just deleted the
zturner updated this revision to Diff 92373.
zturner added a comment.
See what you think about this. I've created a folder called `Mocks` under
`Utility`, and created a new target out of it. `UtilityTests` links against
it, and so does `InterpreterTests`. To do this I had to add the lldb proj
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298325: Delete LLDB's MD5 code. Use LLVM instead. (authored
by zturner).
Changed prior to commit:
https://reviews.llvm.org/D31108?vs=9&id=92405#toc
Repository:
rL LLVM
https://reviews.llvm.org/D
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298334: Delete various lldb FileSystem functions. (authored
by zturner).
Changed prior to commit:
https://reviews.llvm.org/D3?vs=92236&id=92426#toc
Repository:
rL LLVM
https://reviews.llvm.org/D
zturner created this revision.
The only real consumer of this is the `Process` class, so it makes sense (both
conceptually and from a layering standpoint) to hide this in the Process plugin.
This is intended to be NFC.
https://reviews.llvm.org/D31172
Files:
lldb/include/lldb/Core/ArchSpec.h
zturner added a comment.
The architecture help change snuck in, I already removed it locally but didn't
re-upload.
As for why I'm trying to get this out of `ArchSpec`, it turns out `ArchSpec` is
one of the biggest causes of dependency cycles. It's used all over the place,
which triggers a dep
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298465: Delete the remainder of platform specific code in
FileSpec. (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D31129?vs=92373&id=92577#toc
Repository:
rL LLVM
https://
zturner added a comment.
Networking isn't my area of domain expertise, so these are mostly just general
comments.
Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+ std::map m_listen_sockets;
};
Any particular reason you're using a `std::map` instead of
zturner accepted this revision.
zturner added inline comments.
This revision is now accepted and ready to land.
Comment at: include/lldb/Host/SocketAddress.h:44-45
+
//
+ static std::vector GetAddressI
zturner added a comment.
How much would it complicate things to move the hand maintained files out of
tree? If the Xcode build isn't really a thing we're officially supporting,
perhaps we can take that aggressive approach in-tree as well?
Comment at: cmake/modules/LLDBConfig
zturner added inline comments.
Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+ std::map m_listen_sockets;
};
jasonmolenda wrote:
> zturner wrote:
> > Any particular reason you're using a `std::map` instead of a `DenseMap` or
> > other similar LLVM stru
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
No need to request review when removing dead code.
https://reviews.llvm.org/D32148
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
ht
zturner added inline comments.
Comment at: source/Utility/ConstString.cpp:49
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
Why do we even have this fu
zturner added a comment.
If you look at the source code of yaml2obj, all this boils down to a single
call to `ELFState::writeELF`. If you just link against that, we could avoid
even shelling out to an external tool, and the YAML could be a string literal
defined inside the unit test cpp file.
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
Code is still present in history, if someone needs this again in the future
they can do some git archaeology to dig it up.
Repository:
rL LLVM
https://reviews.llvm.org/D32503
zturner accepted this revision.
zturner added a comment.
Alright, thanks for trying anyway!
https://reviews.llvm.org/D32434
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added inline comments.
Comment at: source/Host/common/MainLoop.cpp:135
+
+template void MainLoop::RunImpl::ForEachReadFD(F &&f) {
+ assert(num_events >= 0);
Why do we need this function? Just have a function that returns an
`ArrayRef` and let the call
zturner added inline comments.
Comment at: docs/lldb-gdb-remote.txt:214
+//
+// BRIEF
+// Packet for starting tracing of type lldb::TraceType. The following
I just noticed that none of our documentation uses doxygen? Weird.
Comment at: includ
zturner requested changes to this revision.
zturner added a comment.
I would suggest putting this in LLVM, as something liek this:
namespace llvm {
template
void parallel_apply(Range &&R, Func &&F) {
// enqueue items here.
// wait for all tasks to complete.
}
}
Repository:
r
zturner added a comment.
s/instead of LLVM/instead of LLDB/
Repository:
rL LLVM
https://reviews.llvm.org/D32757
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.
Not to sound like a broken record, but please try to put this in LLVM instead
of LLVM. I suggested a convenient function signature earlier.
Repository:
rL LLVM
https://reviews
zturner requested changes to this revision.
zturner added inline comments.
This revision now requires changes to proceed.
Comment at:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+
zturner added inline comments.
Comment at:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+std::shared_future f;
+ };
+ std::vector loaders(num_to_load);
+ llvm::ThreadPool lo
zturner added inline comments.
Comment at:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+ struct loader {
+DYLDRendezvous::iterator I;
+ModuleSP m;
+std::shared_future f;
+ };
+ std::vector loaders(num_to_load);
+ llvm::ThreadPool lo
zturner added a comment.
https://reviews.llvm.org/D32826
Repository:
rL LLVM
https://reviews.llvm.org/D32597
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ asm volatile ("int3");
+ delay = false;
tberghammer wrote:
> krytarowski wrote:
> > int3 is specific to x86.
> >
> > Some instructions to emit software b
zturner added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1125
+uint64_t extracted_value;
+value.getAsInteger(16, extracted_value);
+
Should you be checking the return value of `getAsInteger` here?
zturner added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
This will work on MSVC and presumably clang. I'm not sure about gcc. Is that
sufficient for your needs? Do y
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:98
+ unsigned int register_id;
+ key_str_ref.getAsInteger(10, register_id);
+
Do you need to check for an error here?
Comment at: un
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:61-63
+string name;
+thread_info->GetValueForKeyAsString("name", name);
+string reason;
jmajors wrote:
> zturner wrote:
> > `StringRef name, reason;`
> The
zturner added a comment.
Getting pretty close. Sorry, still have a few more nits.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:25-30
+ elements["pid"].getAsInteger(16, pid);
+ elements["parent-pid"].getAsInteger(16, parent_pid);
+ elements["real-uid"].ge
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:167
+std::stringstream ss;
+ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+std::string hex_id = ss.str();
jmajors wrote:
> zturner wr
zturner added inline comments.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
labath wrote:
> krytarowski wrote:
> > labath wrote:
> > > krytarowski wrote:
> > > > jmajors wrote:
> > > > > kr
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+ val.getAsInteger(16, stop_reply->thread);
+ key.substr(1, 2).getAsInteger(16, stop_reply->s
zturner created this revision.
The goal here is to eventually delete the `StringConvert` class, as the llvm
APIs for doing string to number conversion have a cleaner syntax.
In order to reduce code churn, since I was in this code anyway, I changed the
deeply nested indentation of the code block
zturner added a reviewer: lhames.
zturner added inline comments.
Comment at: include/lldb/Utility/Status.h:108
+ explicit Status(llvm::Error error);
+ explicit operator llvm::Error();
+
I think we should remove the conversion operator. Instead, why don't we ma
zturner added inline comments.
Comment at: source/Utility/Status.cpp:81-88
+Status::operator llvm::Error() {
+ if (Success())
+return llvm::Error::success();
+ if (m_type == ErrorType::eErrorTypePOSIX)
+return llvm::errorCodeToError(std::error_code(m_code,
std::generic
zturner added a comment.
Mostly just that implicit conversion operators are a very subtle source of
bugs, and you can't even find where they're being used because it's impossible
to grep for it.
If you're signing up to get an object that has strict usage semantics like
`llvm::Error`, you had b
zturner added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
-
zturner added inline comments.
Comment at: source/Utility/Status.cpp:81-88
+Status::operator llvm::Error() {
+ if (Success())
+return llvm::Error::success();
+ if (m_type == ErrorType::eErrorTypePOSIX)
+return llvm::errorCodeToError(std::error_code(m_code,
std::generic
zturner added a comment.
In https://reviews.llvm.org/D32930#767820, @beanz wrote:
> One small comment below. In general I agree with the thoughts here, and I
> think that this is a huge step forward for testing the debug server
> components.
>
> I also agree with Zachary in principal that it wo
zturner added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:738-742
+StartProcessorTracing(tid, m_pt_process_config, traceMonitor);
+if (traceMonitor.get() != nullptr) {
+ traceMonitor->setUserID(m_pt_process_uid);
+ m_pt_trace
zturner added inline comments.
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:148
- ::pid_t Attach(lldb::pid_t pid, Status &error);
+ static llvm::Expected> Attach(::pid_t pid);
Before it was only returning 1, now it's returning a vector. An
zturner added a comment.
The last time I tried to do this we couldn't because it didn't yet work on iOS.
I checked with some Apple people though and they said `thread_local` support
was released last year on all Apple platforms. So hopefully there's no more
hurdles to getting this in.
https
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
Nice, yea this will be a big help I think.
https://reviews.llvm.org/D34400
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lis
zturner added inline comments.
Comment at: unittests/Utility/Helpers/TestUtilities.cpp:17
+
+std::string lldb_private::GetInputFile(llvm::Twine name) {
+ llvm::SmallString<128> result = llvm::sys::path::parent_path(TestMainArgv0);
this should be `const Twine &na
zturner added a comment.
Can you do a diff before and after of the analyze-deps script output? I don't
imagine it will make a difference, but I've been surprised by it before. In
general you should do it before and after every patch of this nature.
https://reviews.llvm.org/D34746
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
Seems fine to me, beanz?
Repository:
rL LLVM
https://reviews.llvm.org/D36886
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http:
zturner added inline comments.
Comment at: source/Host/CMakeLists.txt:166-168
+ if (LIBXML2_FOUND)
+list(APPEND EXTRA_LIBS ${LIBXML2_LIBRARIES})
+ endif()
Even if libxml is found, that doesn't mean the build is configured to use it,
does it? Correct me if
zturner added a comment.
It turns out the function this called, `DispatchInputInterrupt`, already
acquires a mutex. Maybe put the synchronization in there? Then you don't have
to reproduce the synchronization in both MI and LLDB
https://reviews.llvm.org/D37926
zturner added inline comments.
Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
lemo wrote:
> lemo wrote:
> > lemo wrote:
> > > clayborg
zturner added a comment.
Give me a few more hours, if there's a way to make this work with
`line_iterator` I'd really prefer that since I think it improves readability.
Can you confirm that if you were able to write:
auto begin = line_iterator(str, /* skip_empty_lines =*/ false);
auto end
zturner added a comment.
In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
This
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
seems fine to me, I agree with the point about non-determinism, due to the
nature of signals and the fact this is only intended to provide a best-effort
interruption.
https://reviews.llvm.
zturner added inline comments.
Comment at: source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:635-652
const ConstString name(context.m_decl_name.getAsString().c_str());
const char *name_unique_cstr = name.GetCString();
static ConstString id_name("id");
stati
zturner added a comment.
Hi Stephane, what's the status of this? Do you still need this functionality?
Repository:
rL LLVM
https://reviews.llvm.org/D12245
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/ma
zturner added inline comments.
Comment at: source/Utility/DataExtractor.cpp:566
size_t byte_size) const {
- switch (byte_size) {
- case 1:
-return GetU8(offset_ptr);
-break;
- case 2:
-return GetU16(offset_ptr);
-break;
- cas
zturner added inline comments.
Comment at: CMakeLists.txt:15
+# Define the LLDB_CONFIGURATION_xxx matching the build type
+if( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+ add_definitions( -DLLDB_CONFIGURATION_DEBUG )
I'm pretty sure that if you run CMake wit
zturner added inline comments.
Comment at: CMakeLists.txt:15
+# Define the LLDB_CONFIGURATION_xxx matching the build type
+if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+ add_definitions( -DLLDB_CONFIGURATION_RELEASE )
Isn't this identical to the code be
zturner added inline comments.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:90-93
+ // 16 is just a maximum value, query hardware for actual watchpoint count
+ m_max_hwp_supported = 16;
+ m_max_hbp_supported = 16;
+ m_refresh_hwdebug_info =
zturner added a comment.
In https://reviews.llvm.org/D38897#897378, @gut wrote:
> Thanks for supporting this change. I guess @anajuliapc will add you both as
> reviewer as soon as she updates this patch.
>
> BTW, I agree that patches should be improving code quality but I wanted to
> highlight
zturner added a subscriber: lldb-commits.
zturner added a comment.
One possible reason for why this never got any traction is that `lldb-commits`
wasn't added as a subscriber. While it's true that the tagged people should
have chimed in, having the whole commits list will get some more visibili
zturner added a comment.
In https://reviews.llvm.org/D36347#902157, @clayborg wrote:
> Please do convert to python. Just know that you can use "lldb -P" to get the
> python path that is needed in order to do "import lldb" in the python script.
> So you can try doing a "import lldb", and if that
zturner added a comment.
Can you upload diffs with context in the future?
I'm trying to figure out whether `is_exe` is used for anything where
non-existance should not be considered fatal, but I can't see the rest of the
file here.
Repository:
rL LLVM
https://reviews.llvm.org/D39199
___
zturner added a comment.
I just wanted to make sure nobody was *already* relying on that behavior. If
we can get away with being strict, we should be strict.
https://reviews.llvm.org/D39199
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
zturner added a comment.
There is already a CMake variable called `LLDB_TEST_COMPILER`. This
`LLDB_TEST_CLANG` was introduced later, I guess unaware of the presence of
`LLDB_TEST_COMPILER`. Would it be possible to standardize on one variable?
This would also mean deleting the `TEST_C_COMPILE
zturner added a comment.
Yea. Right now there's FOUR different variables to specify the compiler, and
they can all conflict with each other. You can theoretically set
LLDB_TEST_COMPILER=
LLDB_TEST_CLANG=On
LLDB_TEST_C_COMPILER=/usr/bin/cc
LLDB_TEST_CXX_COMPILER=/usr/bin/clang++
This i
zturner added a comment.
In https://reviews.llvm.org/D39215#904578, @labath wrote:
> I've played around with this, but I couldn't get the `lit/lit.site.cfg.in`
> file to see the expanded values of the `$` generator
> expression (the reason it works now is because the file is special-casing
> L
zturner added a comment.
In https://reviews.llvm.org/D39215#904615, @zturner wrote:
> In https://reviews.llvm.org/D39215#904578, @labath wrote:
>
> > I've played around with this, but I couldn't get the `lit/lit.site.cfg.in`
> > file to see the expanded values of the `$` generator
> > expressio
zturner added a comment.
Ok the issue is that you cant use CMake generator expressions in this way.
This should work though:
if (TARGET clang)
set(LLDB_DEFAULT_TEST_COMPILER
"${LLVM_BINARY_DIR}/clang${CMAKE_EXECUTABLE_SUFFIX}")
else()
set(LLDB_DEFAULT_TEST_COMPILER "")
endif()
zturner added a comment.
In https://reviews.llvm.org/D39215#905259, @ted wrote:
> We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb,
> hexagon-clang, etc. The test infrastructure is smart enough to pick up
> hexagon-lldb-mi if we tell it to run with hexagon-lldb using
zturner added a comment.
I know you're doing things the way it's always been done, but I want to start
questioning some long-standing practices :) So I'm not picking on you
specifically, but anywhere we can migrate towards something better
incrementally, I think we should do so.
===
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
labath wrote:
> zturner wrote:
> > Why d
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
zturner wrote:
> labath wrote:
> > clayborg wrote:
> > > zturner wrote:
> > > > Why do we need a `ConstStr
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> One time at startup. No threads contending yet. Asking for plug-in by name is
> made fast for later. I would le
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > One time at startup. No threads contending yet. Asking for plug-in by
zturner accepted this revision.
zturner added a comment.
I will ping them for some numbers and more details of their test setup.
Regardless, I didn't mean to derail the code review. But, I really really want
to reach a point where we can stop falling back on the "we need to be safe even
in th
zturner added a comment.
Do I understand correctly that this will insert breakpoints on *all* clang
diagnostics? That's not necessarily bad, but I was under the impression
originally that it would let you pick the diagnostics you wanted to insert
breakpoints on.
Also, What is the workflow for
zturner added a comment.
A test would be something like this:
// dll.cpp
BOOL WINAPI DllMain(HINSTANCE h, DWORD reason, void* reserved) {
return TRUE;
}
int __declspec(dllexport) DllFunc(int n) {
int x = n * n;
return x; // set breakpoint here
}
// main.cpp
int
401 - 500 of 733 matches
Mail list logo
