Re: [Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via lldb-commits]

I'll have a look, sorry about that.

On Wed, Oct 9, 2019 at 4:37 PM Shafik Yaghmour via Phabricator <
revi...@reviews.llvm.org> wrote:

> shafik added a comment.
>
> This change broek the`TestDataFormatterInvalidStdUniquePtr.py` test, see:
> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2411/testReport/
>
> I verified that reverting this commit fixes the test.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D68641/new/
>
> https://reviews.llvm.org/D68641
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21328: [lldb] Fixed incorrect endianness when evaluating certain expressions

[Cameron via lldb-commits]

cameron314 added a comment.

Hmm, I don't think this is easily testable at all. We happened to see it on our 
(out-of-tree) big-endian architecture when debugging remotely on Windows, but 
only for variables backed by registers. I was thus able to trace the problem 
back to this code, fix it, and verify it locally. But I can't reproduce this 
for other architectures because they likely don't follow the same code path 
(plus I don't have the hardware/simulator setup required). And trying to unit 
test this would be... quite a challenge, I feel, considering all the layers 
involved.


https://reviews.llvm.org/D21328



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

Re: [Lldb-commits] [PATCH] D21328: [lldb] Fixed incorrect endianness when evaluating certain expressions

[Cameron via lldb-commits]

cameron314 added a comment.

@spyffe, do you have a minute today?


https://reviews.llvm.org/D21328



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

Re: [Lldb-commits] [PATCH] D21328: [lldb] Fixed incorrect endianness when evaluating certain expressions

[Cameron via lldb-commits]

cameron314 added a comment.

Sorry to bug you again, but @spyffe, could you have a look when you have a sec?


http://reviews.llvm.org/D21328



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

Re: [Lldb-commits] [PATCH] D21328: [lldb] Fixed incorrect endianness when evaluating certain expressions

[Cameron via lldb-commits]

cameron314 added a comment.

@spyffe, do you have a minute to look this over?


http://reviews.llvm.org/D21328



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

[Lldb-commits] [PATCH] D21328: [lldb] Fixed incorrect endianness when evaluating certain expressions

[Cameron via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: spyffe, zturner, clayborg.
cameron314 added a subscriber: lldb-commits.

The `EntityVariable` materializer was, under certain conditions, taking the 
bytes of a `DataExtractor` that were potentially in host order (e.g. little 
endian) and putting them in the `IRMemoryMap` (which assumes all values are in 
target order, e.g. big endian). This caused certain values to have the wrong 
endianness during expression evaluation.

On our platform, this manifested as certain expressions yielding incorrect 
results when the variables were in registers (e.g. `argc + 1` would give 
`0x0101`).

http://reviews.llvm.org/D21328

Files:
  include/lldb/Expression/IRMemoryMap.h
  source/Expression/IRMemoryMap.cpp
  source/Expression/Materializer.cpp

Index: source/Expression/Materializer.cpp
===
--- source/Expression/Materializer.cpp
+++ source/Expression/Materializer.cpp
@@ -575,7 +575,7 @@
 
 Error write_error;
 
-map.WriteMemory(m_temporary_allocation, data.GetDataStart(), 
data.GetByteSize(), write_error);
+map.WriteMemory(m_temporary_allocation, data.GetDataStart(), 
data.GetByteSize(), data.GetByteOrder(), write_error);
 
 if (!write_error.Success())
 {
Index: source/Expression/IRMemoryMap.cpp
===
--- source/Expression/IRMemoryMap.cpp
+++ source/Expression/IRMemoryMap.cpp
@@ -564,6 +564,22 @@
 }
 
 void
+IRMemoryMap::WriteMemory(lldb::addr_t process_address, const uint8_t *bytes, 
size_t size, lldb::ByteOrder byteOrder, Error &error)
+{
+std::vector temp;
+if (byteOrder != GetByteOrder())
+{
+// Need to byte-swap the bytes before putting them in the map, 
otherwise they'll
+// be interpreted with the wrong endianness when they're read back out.
+temp.reserve(size);
+typedef std::reverse_iterator revit;
+temp.assign(revit(bytes + size), revit(bytes));
+bytes = temp.data();
+}
+WriteMemory(process_address, bytes, size, error);
+}
+
+void
 IRMemoryMap::WriteMemory (lldb::addr_t process_address, const uint8_t *bytes, 
size_t size, Error &error)
 {
 error.Clear();
Index: include/lldb/Expression/IRMemoryMap.h
===
--- include/lldb/Expression/IRMemoryMap.h
+++ include/lldb/Expression/IRMemoryMap.h
@@ -60,6 +60,7 @@
 void Free (lldb::addr_t process_address, Error &error);
 
 void WriteMemory (lldb::addr_t process_address, const uint8_t *bytes, 
size_t size, Error &error);
+void WriteMemory (lldb::addr_t process_address, const uint8_t *bytes, 
size_t size, lldb::ByteOrder order, Error &error);
 void WriteScalarToMemory (lldb::addr_t process_address, Scalar &scalar, 
size_t size, Error &error);
 void WritePointerToMemory (lldb::addr_t process_address, lldb::addr_t 
address, Error &error);
 void ReadMemory (uint8_t *bytes, lldb::addr_t process_address, size_t 
size, Error &error);


Index: source/Expression/Materializer.cpp
===
--- source/Expression/Materializer.cpp
+++ source/Expression/Materializer.cpp
@@ -575,7 +575,7 @@
 
 Error write_error;
 
-map.WriteMemory(m_temporary_allocation, data.GetDataStart(), data.GetByteSize(), write_error);
+map.WriteMemory(m_temporary_allocation, data.GetDataStart(), data.GetByteSize(), data.GetByteOrder(), write_error);
 
 if (!write_error.Success())
 {
Index: source/Expression/IRMemoryMap.cpp
===
--- source/Expression/IRMemoryMap.cpp
+++ source/Expression/IRMemoryMap.cpp
@@ -564,6 +564,22 @@
 }
 
 void
+IRMemoryMap::WriteMemory(lldb::addr_t process_address, const uint8_t *bytes, size_t size, lldb::ByteOrder byteOrder, Error &error)
+{
+std::vector temp;
+if (byteOrder != GetByteOrder())
+{
+// Need to byte-swap the bytes before putting them in the map, otherwise they'll
+// be interpreted with the wrong endianness when they're read back out.
+temp.reserve(size);
+typedef std::reverse_iterator revit;
+temp.assign(revit(bytes + size), revit(bytes));
+bytes = temp.data();
+}
+WriteMemory(process_address, bytes, size, error);
+}
+
+void
 IRMemoryMap::WriteMemory (lldb::addr_t process_address, const uint8_t *bytes, size_t size, Error &error)
 {
 error.Clear();
Index: include/lldb/Expression/IRMemoryMap.h
===
--- include/lldb/Expression/IRMemoryMap.h
+++ include/lldb/Expression/IRMemoryMap.h
@@ -60,6 +60,7 @@
 void Free (lldb::addr_t process_address, Error &error);
 
 void WriteMemory (lldb::addr_t process_address, const

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL272682: [lldb] Fixed race conditions on private state thread 
exit (authored by cameron314).

Changed prior to commit:
  http://reviews.llvm.org/D21296?vs=60536&id=60693#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D21296

Files:
  lldb/trunk/include/lldb/Target/Process.h
  lldb/trunk/source/Target/Process.cpp

Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -4088,7 +4088,7 @@
 void
 Process::StopPrivateStateThread ()
 {
-if (PrivateStateThreadIsValid ())
+if (m_private_state_thread.IsJoinable ())
 ControlPrivateStateThread (eBroadcastInternalStateControlStop);
 else
 {
@@ -4110,21 +4110,23 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Broadcast the event.
+// It is important to do this outside of the if below, because
+// it's possible that the thread state is invalid but that the
+// thread is waiting on a control event instead of simply being
+// on its way out (this should not happen, but it apparently can).
+if (log)
+log->Printf ("Sending control event of type: %d.", signal);
+std::shared_ptr event_receipt_sp(new EventDataReceipt());
+m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+
+// Wait for the event receipt or for the private state thread to exit
+bool receipt_received = false;
+if (PrivateStateThreadIsValid())
 {
-if (log)
-log->Printf ("Sending control event of type: %d.", signal);
-// Send the control event and wait for the receipt or for the private state
-// thread to exit
-std::shared_ptr event_receipt_sp(new EventDataReceipt());
-m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
-
-bool receipt_received = false;
 while (!receipt_received)
 {
 bool timed_out = false;
@@ -4137,23 +4139,24 @@
 if (!receipt_received)
 {
 // Check if the private state thread is still around. If it isn't then we are done waiting
-if (!m_private_state_thread.IsJoinable())
-break; // Private state thread exited, we are done
+if (!PrivateStateThreadIsValid())
+break; // Private state thread exited or is exiting, we are done
 }
 }
-
-if (signal == eBroadcastInternalStateControlStop)
-{
-thread_result_t result = NULL;
-private_state_thread.Join(&result);
-}
 }
-else
+
+if (signal == eBroadcastInternalStateControlStop)
 {
-if (log)
-log->Printf ("Private state thread already dead, no need to signal it to stop.");
+thread_result_t result = NULL;
+m_private_state_thread.Join(&result);
+m_private_state_thread.Reset();
 }
 }
+else
+{
+if (log)
+log->Printf("Private state thread already dead, no need to signal it to stop.");
+}
 }
 
 void
@@ -4446,7 +4449,6 @@
 // try to change it on the way out.
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: lldb/trunk/include/lldb/Target/Process.h
===
--- lldb/trunk/include/lldb/Target/Process.h
+++ lldb/trunk/include/lldb/Target/Process.h
@@ -3309,9 +3309,13 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateInvalid &&
+   state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
-
+
 void
 ForceNextEventDelivery()
 {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

cameron314 added a comment.

Thanks everyone :-)

Ah, yeah, sorry if I gave the wrong impression, but that comment is not 
specific to Linux (in fact, I've only seen it once, on Windows). At one point 
the debugger had entered ControlPrivateStateThread on one thread to stop it, 
seen that the thread was already in an invalid state (it was), and assumed that 
meant that the thread was already exiting and did a join without sending the 
stop. But the state thread somehow wasn't on its way out yet, it was stuck 
waiting for a control event first (this is the part that I'm not sure should be 
possible, but empirically is). This caused a deadlock. So I changed my patch to 
always send the event if the thread is joinable, not just if its state is 
valid, and left that comment to explain why this must remain so.


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

cameron314 added a comment.

@clayborg: Thanks for having a look! I've added Jim Ingham as a reviewer. 
@jingham, I'd appreciate if you could take a few minutes to look this over.

Right, I'd seen the backup/restore of the thread. As far as I can tell it 
should still work; the code in `ControlPrivateStateThread` has no idea it's 
working with a temporary thread, just as it didn't know before (in fact, if you 
look carefully at the code in the present tip of the trunk, a recent change 
seems to have introduced a mix of using both `private_state_thread` and 
`m_private_state_thread`, probably by accident). `m_private_state_thread` 
cannot be reset to the backup during a control event, since the first thing 
that's done before restoring the backup thread is to stop the temporary thread.


http://reviews.llvm.org/D21296



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

[Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: clayborg, labath, zturner.
cameron314 added a subscriber: lldb-commits.

This is the follow-up to D19122, which was accepted but subsequently reverted 
due to a bug it introduced (that I didn't see during local testing on Windows 
but which manifested quite often on Linux). That bug (a race between the 
Process object was being destroyed and the thread terminating, caused by the 
join not being done under certain conditions) is fixed in this version of the 
patch.

This patch fixes various races between the time the private state thread is 
signaled to exit and the time it actually exits (during which it no longer 
responds to events). Previously, this was consistently causing 2-second timeout 
delays on process detach/stop for us.

This also prevents crashes that were caused by the thread controlling its own 
owning pointer while the controller was using it (copying the thread wrapper is 
not enough to mitigate this, since the internal thread object was getting reset 
anyway). Again, we were seeing this consistently.

For what it's worth, I've run the test suite with this change (on Linux) 
without any regressions, and the number of reruns dropped from 15 to 0 for me 
(though that last part may be coincidence).

http://reviews.llvm.org/D21296

Files:
  include/lldb/Target/Process.h
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4088,7 +4088,7 @@
 void
 Process::StopPrivateStateThread ()
 {
-if (PrivateStateThreadIsValid ())
+if (m_private_state_thread.IsJoinable ())
 ControlPrivateStateThread (eBroadcastInternalStateControlStop);
 else
 {
@@ -4110,21 +4110,23 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
-{
-if (log)
-log->Printf ("Sending control event of type: %d.", signal);
-// Send the control event and wait for the receipt or for the private state
-// thread to exit
-std::shared_ptr event_receipt_sp(new EventDataReceipt());
-m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+// Broadcast the event.
+// It is important to do this outside of the if below, because
+// it's possible that the thread state is invalid but that the
+// thread is waiting on a control event instead of simply being
+// on its way out (this should not happen, but it apparently can).
+if (log)
+log->Printf ("Sending control event of type: %d.", signal);
+std::shared_ptr event_receipt_sp(new EventDataReceipt());
+m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
 
-bool receipt_received = false;
+// Wait for the event receipt or for the private state thread to exit
+bool receipt_received = false;
+if (PrivateStateThreadIsValid())
+{
 while (!receipt_received)
 {
 bool timed_out = false;
@@ -4137,23 +4139,24 @@
 if (!receipt_received)
 {
 // Check if the private state thread is still around. If it isn't then we are done waiting
-if (!m_private_state_thread.IsJoinable())
-break; // Private state thread exited, we are done
+if (!PrivateStateThreadIsValid())
+break; // Private state thread exited or is exiting, we are done
 }
 }
-
-if (signal == eBroadcastInternalStateControlStop)
-{
-thread_result_t result = NULL;
-private_state_thread.Join(&result);
-}
 }
-else
+
+if (signal == eBroadcastInternalStateControlStop)
 {
-if (log)
-log->Printf ("Private state thread already dead, no need to signal it to stop.");
+thread_result_t result = NULL;
+m_private_state_thread.Join(&result);
+m_private_state_thread.Reset();
 }
 }
+else
+{
+if (log)
+log->Printf("Private state thread already dead, no need to signal it to stop.");
+}
 }
 
 void
@@ -4446,7 +4449,6 @@
 // try to change it on the way out.
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
-m_private_state_thread.Reset();
 return NULL;

Re: [Lldb-commits] [PATCH] D20738: Make sure that indexing is done before clearing DIE info.

[Cameron via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL271209: [LLDB] Make sure that indexing is done before 
clearing DIE info (authored by cameron314).

Changed prior to commit:
  http://reviews.llvm.org/D20738?vs=58661&id=58965#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20738

Files:
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2162,15 +2162,19 @@
 if (debug_info)
 {
 const uint32_t num_compile_units = GetNumCompileUnits();
+if (num_compile_units == 0)
+return;
+
 std::vector function_basename_index(num_compile_units);
 std::vector function_fullname_index(num_compile_units);
 std::vector function_method_index(num_compile_units);
 std::vector function_selector_index(num_compile_units);
 std::vector objc_class_selectors_index(num_compile_units);
 std::vector global_index(num_compile_units);
 std::vector type_index(num_compile_units);
 std::vector namespace_index(num_compile_units);
-
+
+std::vector clear_cu_dies(num_compile_units, false);
 auto parser_fn = [this,
   debug_info,
   &function_basename_index,
@@ -2183,25 +2187,62 @@
   &namespace_index](uint32_t cu_idx)
 {
 DWARFCompileUnit* dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);
-bool clear_dies = dwarf_cu->ExtractDIEsIfNeeded(false) > 1;
-
-dwarf_cu->Index(function_basename_index[cu_idx],
-function_fullname_index[cu_idx],
-function_method_index[cu_idx],
-function_selector_index[cu_idx],
-objc_class_selectors_index[cu_idx],
-global_index[cu_idx],
-type_index[cu_idx],
-namespace_index[cu_idx]);
-
-// Keep memory down by clearing DIEs if this generate function
-// caused them to be parsed
-if (clear_dies)
-dwarf_cu->ClearDIEs(true);
-
+if (dwarf_cu)
+{
+dwarf_cu->Index(function_basename_index[cu_idx],
+function_fullname_index[cu_idx],
+function_method_index[cu_idx],
+function_selector_index[cu_idx],
+objc_class_selectors_index[cu_idx],
+global_index[cu_idx],
+type_index[cu_idx],
+namespace_index[cu_idx]);
+}
 return cu_idx;
 };
 
+auto extract_fn = [this,
+   debug_info,
+   num_compile_units](uint32_t cu_idx)
+{
+DWARFCompileUnit* dwarf_cu = debug_info->GetCompileUnitAtIndex(cu_idx);
+if (dwarf_cu)
+{
+// dwarf_cu->ExtractDIEsIfNeeded(false) will return zero if the
+// DIEs for a compile unit have already been parsed.
+return std::make_pair(cu_idx, dwarf_cu->ExtractDIEsIfNeeded(false) > 1);
+}
+return std::make_pair(cu_idx, false);
+};
+
+// Create a task runner that extracts dies for each DWARF compile unit in a separate thread
+TaskRunner> task_runner_extract;
+for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
+task_runner_extract.AddTask(extract_fn, cu_idx);
+
+//--
+// First figure out which compile units didn't have their DIEs already
+// parsed and remember this.  If no DIEs were parsed prior to this index
+// function call, we are going to want to clear the CU dies after we
+// are done indexing to make sure we don't pull in all DWARF dies, but
+// we need to wait until all compile units have been indexed in case
+// a DIE in one compile unit refers to another and the indexes accesses
+// those DIEs.
+//--
+while (true)
+{
+auto f = task_runner_extract.WaitForNextCompletedTask();
+if (!f.valid())
+break;
+unsigned cu_idx;
+bool clear;
+std::tie(cu_idx, clear) = f.get();
+clear_cu_dies[cu_idx] = clear;
+}
+
+// Now create a task runner that can index each DWARF compile unit in a sepa

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

I added the makefile with r269366. The tests seem to be working now:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14641
Sorry about that!

On Thu, May 12, 2016 at 6:07 PM, Cameron  wrote:

> Thanks Jim, I'll definitely use that as a template next time!
>
> Ah, I think I found the problem with the test. The makefile was in the
> patch but wasn't committed. Trying that out now.
>
> On Thu, May 12, 2016 at 5:45 PM, Jim Ingham  wrote:
>
>>
>> > On May 12, 2016, at 2:25 PM, Cameron  wrote:
>> >
>> > Sorry to break the build! Apparently 'make clean' isn't executing
>> cleanly during the build step of the test, but I haven't the faintest idea
>> why. It builds/runs fine locally for me (then again, I'm on Windows). The
>> makefile is dead simple, and is identical to that of some other tests. Has
>> anyone seen something like this before?
>> >
>> > Ah, I would have written a test using the APIs if I knew. I didn't see
>> any other similar tests that set up LLDB from scratch without going through
>> the command line. For reference, can you point me to one of these tests I
>> can use as an example for the next time?
>>
>> expression_command/fixits/TestFixIts.py is one.  It makes a target, runs
>> it hits breakpoints and does some other stuff.  Most of the Python API
>> tests start by creating the target in Python - whereas the command-line
>> tests tend to use the file command.  So you can find lots of examples by
>> searching for the string "self.dbg.CreateTarget" in all the .py files.
>>
>> Jim
>>
>> >
>> > On Thu, May 12, 2016 at 5:17 PM, Jim Ingham  wrote:
>> > Note that while adding a "expr --allow-jit" flag to control this was
>> great, there already was an SBExpressionOptions option and the appropriate
>> flags available for this, so it was testable.  I was just checking because
>> there really shouldn't be anything we can do from a command that we can't
>> do from the SB API's, but in this case the thing was already in the
>> expression options.
>> >
>> > We do prefer not to write tests with the command line when you can do
>> it with the Python API's.  In our experience, command line tests have
>> tended to be more fragile.  It's not necessary to change the test, there
>> are plenty of other command line tests lying around.  Just for future
>> reference...
>> >
>> > Jim
>> >
>> > > On May 9, 2016, at 1:01 PM, Sean Callanan via lldb-commits <
>> lldb-commits@lists.llvm.org> wrote:
>> > >
>> > > spyffe accepted this revision.
>> > > spyffe added a comment.
>> > > This revision is now accepted and ready to land.
>> > >
>> > > This patch is fine.  I especially appreciate the time you took making
>> the IRInterpreter properly testable.  I will use this myself.
>> > >
>> > >
>> > > 
>> > > Comment at: source/Commands/CommandObjectExpression.cpp:67
>> > > @@ -67,1 +66,3 @@
>> > > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "top-level",
>> 'p', OptionParser::eNoArgument  , NULL, NULL, 0, eArgTypeNone,
>>  "Interpret the expression as top-level definitions rather than code to be
>> immediately executed."},
>> > > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "allow-jit",
>> 'j', OptionParser::eRequiredArgument, nullptr, nullptr, 0,
>> eArgTypeBoolean,"Controls whether the expression can fall back to being
>> JITted if it's not supported by the interpreter (defaults to true)."}
>> > > };
>> > > 
>> > > This is a great feature which I will use in test cases.
>> > >
>> > > 
>> > > Comment at: source/Expression/IRInterpreter.cpp:1102
>> > > @@ -1058,1 +1101,3 @@
>> > > +}
>> > > +break;
>> > > case Instruction::GetElementPtr:
>> > > 
>> > > Looks all right to me.  Thank you!
>> > >
>> > >
>> > > http://reviews.llvm.org/D19124
>> > >
>> > >
>> > >
>> > > ___
>> > > lldb-commits mailing list
>> > > lldb-commits@lists.llvm.org
>> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>> >
>> >
>>
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

Thanks Jim, I'll definitely use that as a template next time!

Ah, I think I found the problem with the test. The makefile was in the
patch but wasn't committed. Trying that out now.

On Thu, May 12, 2016 at 5:45 PM, Jim Ingham  wrote:

>
> > On May 12, 2016, at 2:25 PM, Cameron  wrote:
> >
> > Sorry to break the build! Apparently 'make clean' isn't executing
> cleanly during the build step of the test, but I haven't the faintest idea
> why. It builds/runs fine locally for me (then again, I'm on Windows). The
> makefile is dead simple, and is identical to that of some other tests. Has
> anyone seen something like this before?
> >
> > Ah, I would have written a test using the APIs if I knew. I didn't see
> any other similar tests that set up LLDB from scratch without going through
> the command line. For reference, can you point me to one of these tests I
> can use as an example for the next time?
>
> expression_command/fixits/TestFixIts.py is one.  It makes a target, runs
> it hits breakpoints and does some other stuff.  Most of the Python API
> tests start by creating the target in Python - whereas the command-line
> tests tend to use the file command.  So you can find lots of examples by
> searching for the string "self.dbg.CreateTarget" in all the .py files.
>
> Jim
>
> >
> > On Thu, May 12, 2016 at 5:17 PM, Jim Ingham  wrote:
> > Note that while adding a "expr --allow-jit" flag to control this was
> great, there already was an SBExpressionOptions option and the appropriate
> flags available for this, so it was testable.  I was just checking because
> there really shouldn't be anything we can do from a command that we can't
> do from the SB API's, but in this case the thing was already in the
> expression options.
> >
> > We do prefer not to write tests with the command line when you can do it
> with the Python API's.  In our experience, command line tests have tended
> to be more fragile.  It's not necessary to change the test, there are
> plenty of other command line tests lying around.  Just for future
> reference...
> >
> > Jim
> >
> > > On May 9, 2016, at 1:01 PM, Sean Callanan via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> > >
> > > spyffe accepted this revision.
> > > spyffe added a comment.
> > > This revision is now accepted and ready to land.
> > >
> > > This patch is fine.  I especially appreciate the time you took making
> the IRInterpreter properly testable.  I will use this myself.
> > >
> > >
> > > 
> > > Comment at: source/Commands/CommandObjectExpression.cpp:67
> > > @@ -67,1 +66,3 @@
> > > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "top-level",
> 'p', OptionParser::eNoArgument  , NULL, NULL, 0, eArgTypeNone,
>  "Interpret the expression as top-level definitions rather than code to be
> immediately executed."},
> > > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "allow-jit",
> 'j', OptionParser::eRequiredArgument, nullptr, nullptr, 0,
> eArgTypeBoolean,"Controls whether the expression can fall back to being
> JITted if it's not supported by the interpreter (defaults to true)."}
> > > };
> > > 
> > > This is a great feature which I will use in test cases.
> > >
> > > 
> > > Comment at: source/Expression/IRInterpreter.cpp:1102
> > > @@ -1058,1 +1101,3 @@
> > > +}
> > > +break;
> > > case Instruction::GetElementPtr:
> > > 
> > > Looks all right to me.  Thank you!
> > >
> > >
> > > http://reviews.llvm.org/D19124
> > >
> > >
> > >
> > > ___
> > > lldb-commits mailing list
> > > lldb-commits@lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> >
> >
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

Sorry to break the build! Apparently 'make clean' isn't executing cleanly
during the build step of the test, but I haven't the faintest idea why. It
builds/runs fine locally for me (then again, I'm on Windows). The makefile
is dead simple, and is identical to that of some other tests. Has anyone
seen something like this before?

Ah, I would have written a test using the APIs if I knew. I didn't see any
other similar tests that set up LLDB from scratch without going through the
command line. For reference, can you point me to one of these tests I can
use as an example for the next time?

On Thu, May 12, 2016 at 5:17 PM, Jim Ingham  wrote:

> Note that while adding a "expr --allow-jit" flag to control this was
> great, there already was an SBExpressionOptions option and the appropriate
> flags available for this, so it was testable.  I was just checking because
> there really shouldn't be anything we can do from a command that we can't
> do from the SB API's, but in this case the thing was already in the
> expression options.
>
> We do prefer not to write tests with the command line when you can do it
> with the Python API's.  In our experience, command line tests have tended
> to be more fragile.  It's not necessary to change the test, there are
> plenty of other command line tests lying around.  Just for future
> reference...
>
> Jim
>
> > On May 9, 2016, at 1:01 PM, Sean Callanan via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > spyffe accepted this revision.
> > spyffe added a comment.
> > This revision is now accepted and ready to land.
> >
> > This patch is fine.  I especially appreciate the time you took making
> the IRInterpreter properly testable.  I will use this myself.
> >
> >
> > 
> > Comment at: source/Commands/CommandObjectExpression.cpp:67
> > @@ -67,1 +66,3 @@
> > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "top-level",
> 'p', OptionParser::eNoArgument  , NULL, NULL, 0, eArgTypeNone,
>  "Interpret the expression as top-level definitions rather than code to be
> immediately executed."},
> > +{ LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "allow-jit",
> 'j', OptionParser::eRequiredArgument, nullptr, nullptr, 0,
> eArgTypeBoolean,"Controls whether the expression can fall back to being
> JITted if it's not supported by the interpreter (defaults to true)."}
> > };
> > 
> > This is a great feature which I will use in test cases.
> >
> > 
> > Comment at: source/Expression/IRInterpreter.cpp:1102
> > @@ -1058,1 +1101,3 @@
> > +}
> > +break;
> > case Instruction::GetElementPtr:
> > 
> > Looks all right to me.  Thank you!
> >
> >
> > http://reviews.llvm.org/D19124
> >
> >
> >
> > ___
> > lldb-commits mailing list
> > lldb-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

Excellent, thank you!


http://reviews.llvm.org/D19124



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

@spyffe, do you have time to look at this sometime this week? It's a very 
simple patch (the test code took longer to write than the patch itself). Thanks!


http://reviews.llvm.org/D19124



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

All right, no worries :-) I didn't want to bother anyone.


http://reviews.llvm.org/D19124



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

I'd appreciate if someone could take a look at this when they have a moment.


http://reviews.llvm.org/D19124



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

Hmm, that's not good.
No, I don't have a Linux machine set up. I can set up a VM but if you already 
have the dumps/logs it would be faster to start with those.

Thanks!


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 54134.
cameron314 added a comment.

I've added a test for this patch.


http://reviews.llvm.org/D19124

Files:
  packages/Python/lldbsuite/test/expression_command/ir-interpreter/Makefile
  
packages/Python/lldbsuite/test/expression_command/ir-interpreter/TestIRInterpreter.py
  packages/Python/lldbsuite/test/expression_command/ir-interpreter/main.cpp
  source/Commands/CommandObjectExpression.cpp
  source/Commands/CommandObjectExpression.h
  source/Expression/IRInterpreter.cpp

Index: source/Expression/IRInterpreter.cpp
===
--- source/Expression/IRInterpreter.cpp
+++ source/Expression/IRInterpreter.cpp
@@ -106,6 +106,7 @@
 DataLayout &m_target_data;
 lldb_private::IRExecutionUnit  &m_execution_unit;
 const BasicBlock   *m_bb;
+const BasicBlock   *m_prev_bb;
 BasicBlock::const_iterator  m_ii;
 BasicBlock::const_iterator  m_ie;
 
@@ -121,7 +122,9 @@
lldb::addr_t stack_frame_bottom,
lldb::addr_t stack_frame_top) :
 m_target_data (target_data),
-m_execution_unit (execution_unit)
+m_execution_unit (execution_unit),
+m_bb (nullptr),
+m_prev_bb (nullptr)
 {
 m_byte_order = (target_data.isLittleEndian() ? lldb::eByteOrderLittle : lldb::eByteOrderBig);
 m_addr_byte_size = (target_data.getPointerSize(0));
@@ -137,6 +140,7 @@
 
 void Jump (const BasicBlock *bb)
 {
+m_prev_bb = m_bb;
 m_bb = bb;
 m_ii = m_bb->begin();
 m_ie = m_bb->end();
@@ -562,6 +566,7 @@
 case Instruction::Alloca:
 case Instruction::BitCast:
 case Instruction::Br:
+case Instruction::PHI:
 break;
 case Instruction::Call:
 {
@@ -1055,6 +1060,46 @@
 }
 }
 continue;
+case Instruction::PHI:
+{
+const PHINode *phi_inst = dyn_cast(inst);
+
+if (!phi_inst)
+{
+if (log)
+log->Printf("getOpcode() returns PHI, but instruction is not a PHINode");
+error.SetErrorToGenericError();
+error.SetErrorString(interpreter_internal_error);
+return false;
+}
+if (!frame.m_prev_bb)
+{
+if (log)
+log->Printf("Encountered PHI node without having jumped from another basic block");
+error.SetErrorToGenericError();
+error.SetErrorString(interpreter_internal_error);
+return false;
+}
+
+Value* value = phi_inst->getIncomingValueForBlock(frame.m_prev_bb);
+lldb_private::Scalar result;
+if (!frame.EvaluateValue(result, value, module))
+{
+if (log)
+log->Printf("Couldn't evaluate %s", PrintValue(value).c_str());
+error.SetErrorToGenericError();
+error.SetErrorString(bad_value_error);
+return false;
+}
+frame.AssignValue(inst, result, module);
+
+if (log)
+{
+log->Printf("Interpreted a %s", inst->getOpcodeName());
+log->Printf("  Incoming value : %s", frame.SummarizeValue(value).c_str());
+}
+}
+break;
 case Instruction::GetElementPtr:
 {
 const GetElementPtrInst *gep_inst = dyn_cast(inst);
Index: source/Commands/CommandObjectExpression.h
===
--- source/Commands/CommandObjectExpression.h
+++ source/Commands/CommandObjectExpression.h
@@ -57,6 +57,7 @@
 booltop_level;
 boolunwind_on_error;
 boolignore_breakpoints;
+boolallow_jit;
 boolshow_types;
 boolshow_summary;
 booldebug;
Index: source/Commands/CommandObjectExpression.cpp
===
--- source/Commands/CommandObjectExpression.cpp
+++ source/Commands/CommandObjectExpression.cpp
@@ -63,7 +63,8 @@
 { LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "language",   'l', OptionParser::eRequiredArgument, nullptr, nullptr, 0, eArgTypeLanguage,   "Specifies the Language to use when parsing the expression.  If not set the target.language setting is used." },
 { LLDB_OPT_SET_1 | LLDB_OPT_SET_2, false, "apply-fixits",   'X', OptionParser::eRequiredA

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 54098.
cameron314 added a comment.

Oops, uploaded wrong diff a minute ago. Here's the one with the full fix.


http://reviews.llvm.org/D19122

Files:
  include/lldb/Target/Process.h
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, 
cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
 
 void


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
 
 void
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 updated the summary for this revision.
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 54095.
cameron314 added a comment.

I've updated the diff to include a fix for the race between Detach and Stop. 
This has been working nicely for us without any problems for the last few days.

I looked again at putting back the wrapper for the m_private_state_thread, but 
it really doesn't make sense to me. Anything that can go wrong without the 
wrapper can still happen with the wrapper.


http://reviews.llvm.org/D19122

Files:
  include/lldb/Target/Process.h
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, 
cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
 
 void


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
 
 void
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

Ooh, that might work. But when ControlProvateStateThread resets 
m_private_state_control_wait to false there's still a race between that and the 
thread exiting. It could then be set back to false even after the thread has 
exited (this is even likely for a detach).

I tried a different approach, changing `PrivateStateThreadIsValid` to the 
following. I'm not sure it's the right thing to do, but it certainly fixes the 
race (and thus timeout) on our platform:

  bool
  PrivateStateThreadIsValid () const
  {
  lldb::StateType state = m_private_state.GetValue();
  return state != lldb::eStateDetached &&
 state != lldb::eStateExited &&
 m_private_state_thread.IsJoinable();
  }


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

OK, I'll pare down the patch to just that line then.

I found the reason for the time out. At the end of our debug session, we 
destroy the Target, which destroys the process. Process::Destroy in turn calls 
Process::Detach, which calls DoDetach just before calling 
StopPrivateStateThread (which times out). Our platform's process object 
(modeled after the GDB remote one) sets the thread state to eStateDetached in 
its DoDetach implementation; and the private state thread exits as soon as it 
sees that state, meaning it's no longer around handle the 
eBroadcastInternalStateControlStop state event from the StopPrivateStateThread 
just after.

I think the real bug is in StopPrivateStateThread, which only sends the stop 
state if the thread is still joinable; since I removed the Reset, it is of 
course still joinable after it exits. But even with the Reset (which we agree 
shouldn't be there), there is still a race between the time the detach signal 
is sent and when the thread sees it and exits. It seems once a detach or stop 
is sent, no further detaches/stops can be sent, regardless of what the thread 
is doing (since it will exit as soon as it sees the first one). So there should 
be a flag to that effect somewhere checked in ControlPrivateStateThread. 
Probably all the calls to `IsJoinable()` should be replaced with calls to 
`PrivateStateThreadIsValid`, which should be updated to check that flag.

I don't see a nice way to add this flag for each private state thread, though, 
instead of for the process object as a whole. But maybe that's all that's 
necessary?


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

I read the same docs :D This is the important part:

> If multiple threads of execution access the same shared_ptr without 
> synchronization and any of those accesses uses a non-const member function of 
> shared_ptr then a data race will occur; the shared_ptr overloads of atomic 
> functions can be used to prevent the data race.


Copying the shared pointer and accessing its pointee through it is thread safe 
(copying a wrapper object technically isn't, but it will boil down to the same 
thing, so let's leave that aside). But copying it while the pointer is being 
reassigned (`operator=`) on a different thread is //not// thread safe.

To answer your question, yes, absolutely, we can just remove that one call to 
Reset. But if the copy is still necessary, that means a lot of existing code 
(including that copy) is also racy and should be reviewed (though this would 
make sense to do in a separate patch).


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

Hmm, interesting. Yes, it should not be timing out in the first place. That's 
likely a bug on our side, still to be determined. I'm looking into it. 
Regardless, it led us to find and fix this race :-)

I think it still makes sense for LLDB to do a `Cancel()` as a last-ditch effort 
on timing out. But `Cancel()` is fundamentally very dangerous, so then again, 
maybe not.

Can the code that spins up the extra thread (RunThreadPlan) run on a different 
thread than ControlPrivateStateThread runs on? I was under the (perhaps 
entirely false!) impression that the Process object was used by only one thread 
at a time (with the exception of the private state thread(s), of course, but 
that thread shouldn't touch its own control variables).

If that's not the case, then looking at the RunThreadPlan code, there's lots 
more races on m_private_state_thread alone (e.g. calling EqualsThread on it 
while it could be assigned from another thread). And even just creating a copy 
is not thread safe, for example, since it could be reassigned from another 
thread at the same time... you'd need `m_private_state_thread` to be wrap its 
shared pointer with `std::atomic`.


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added a comment.

Note also that using a copy of the instance variable is no different from using 
the instance variable directly, here, since as you say they both have a shared 
pointer to the same underlying object which is manipulated through either of 
the two to the same effect.


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: source/Target/Process.cpp:4116
@@ -4120,1 +4115,3 @@
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {

clayborg wrote:
> If you are going to do IsJoinable(), Cancel(), and Join() then this is still 
> racy in that someone else could do the same thing on another thread. So this 
> code should probably take a mutex to protect:
> 
> ```
> Mutex::Locker locker(m_private_state_thread.GetMutex());
> if (m_private_state_thread.IsJoinable())
> ```
> 
> This would need to live in HostNativeThreadBase because HostThread contains a 
> shared pointer to a HostNativeThreadBase and HostThread can be copied.
> 
I removed the call to `Reset` from the other thread (line 4452 below). It 
didn't make sense for a thread to reset its own `HostThread` since it's not its 
own owner, the Process is.

So now only the `Process` is allowed to control the `HostThread` it owns, and 
there's no races and thus no need for a mutex.


Repository:
  rL LLVM

http://reviews.llvm.org/D19122



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

Whoops, my fault. I was accidentally using the VS2013 command prompt with the 
2015 cmake files. Compiling happily as we speak, test coming up soon after.


Repository:
  rL LLVM

http://reviews.llvm.org/D19124



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

Re: [Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 added a comment.

Ah, that's a bit tricky at the moment. The LLVM tip no longer compiles with 
VS2015 (specifically lib\Support\SourceMgr.cpp), and my Python setup for VS2013 
is all wonky.
This will have to wait a bit.


Repository:
  rL LLVM

http://reviews.llvm.org/D19124



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

[Lldb-commits] [PATCH] D19124: [LLDB] Added support for PHI nodes to IR interpreter

[Cameron via lldb-commits]

cameron314 created this revision.
cameron314 added a reviewer: spyffe.
cameron314 added subscribers: mamai, lldb-commits.
cameron314 set the repository for this revision to rL LLVM.

This allows expressions such as 'i == 1 || i == 2` to be executed using the IR 
interpreter.

Repository:
  rL LLVM

http://reviews.llvm.org/D19124

Files:
  source/Expression/IRInterpreter.cpp

Index: source/Expression/IRInterpreter.cpp
===
--- source/Expression/IRInterpreter.cpp
+++ source/Expression/IRInterpreter.cpp
@@ -106,6 +106,7 @@
 DataLayout &m_target_data;
 lldb_private::IRExecutionUnit  &m_execution_unit;
 const BasicBlock   *m_bb;
+const BasicBlock   *m_prev_bb;
 BasicBlock::const_iterator  m_ii;
 BasicBlock::const_iterator  m_ie;
 
@@ -121,7 +122,9 @@
lldb::addr_t stack_frame_bottom,
lldb::addr_t stack_frame_top) :
 m_target_data (target_data),
-m_execution_unit (execution_unit)
+m_execution_unit (execution_unit),
+m_bb (nullptr),
+m_prev_bb (nullptr)
 {
 m_byte_order = (target_data.isLittleEndian() ? lldb::eByteOrderLittle 
: lldb::eByteOrderBig);
 m_addr_byte_size = (target_data.getPointerSize(0));
@@ -137,6 +140,7 @@
 
 void Jump (const BasicBlock *bb)
 {
+m_prev_bb = m_bb;
 m_bb = bb;
 m_ii = m_bb->begin();
 m_ie = m_bb->end();
@@ -563,6 +567,7 @@
 case Instruction::Alloca:
 case Instruction::BitCast:
 case Instruction::Br:
+case Instruction::PHI:
 break;
 case Instruction::Call:
 {
@@ -1052,6 +1057,46 @@
 }
 }
 continue;
+case Instruction::PHI:
+{
+const PHINode *phi_inst = dyn_cast(inst);
+
+if (!phi_inst)
+{
+if (log)
+log->Printf("getOpcode() returns PHI, but instruction 
is not a PHINode");
+error.SetErrorToGenericError();
+error.SetErrorString(interpreter_internal_error);
+return false;
+}
+if (!frame.m_prev_bb)
+{
+if (log)
+log->Printf("Encountered PHI node without having 
jumped from another basic block");
+error.SetErrorToGenericError();
+error.SetErrorString(interpreter_internal_error);
+return false;
+}
+
+Value* value = 
phi_inst->getIncomingValueForBlock(frame.m_prev_bb);
+lldb_private::Scalar result;
+if (!frame.EvaluateValue(result, value, module))
+{
+if (log)
+log->Printf("Couldn't evaluate %s", 
PrintValue(value).c_str());
+error.SetErrorToGenericError();
+error.SetErrorString(bad_value_error);
+return false;
+}
+frame.AssignValue(inst, result, module);
+
+if (log)
+{
+log->Printf("Interpreted a %s", inst->getOpcodeName());
+log->Printf("  Incoming value : %s", 
frame.SummarizeValue(value).c_str());
+}
+}
+break;
 case Instruction::GetElementPtr:
 {
 const GetElementPtrInst *gep_inst = 
dyn_cast(inst);


Index: source/Expression/IRInterpreter.cpp
===
--- source/Expression/IRInterpreter.cpp
+++ source/Expression/IRInterpreter.cpp
@@ -106,6 +106,7 @@
 DataLayout &m_target_data;
 lldb_private::IRExecutionUnit  &m_execution_unit;
 const BasicBlock   *m_bb;
+const BasicBlock   *m_prev_bb;
 BasicBlock::const_iterator  m_ii;
 BasicBlock::const_iterator  m_ie;
 
@@ -121,7 +122,9 @@
lldb::addr_t stack_frame_bottom,
lldb::addr_t stack_frame_top) :
 m_target_data (target_data),
-m_execution_unit (execution_unit)
+m_execution_unit (execution_unit),
+m_bb (nullptr),
+m_prev_bb (nullptr)
 {
 m_byte_order = (target_data.isLittleEndian() ? lldb::eByteOrderLittle : lldb::eByteOrderBig);
 m_addr_byte_size = (target_data.getPointerSize(0));
@@ -137,6 +140,7 @@
 
 void Jump (const BasicBlock *bb)
 {
+m_prev_bb = m_bb;
 m_bb = bb;
 m_ii = m_bb->begin();
 m_ie = m_bb->end();
@@ -563,6 +567,7 @@
 case Instructi

[Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread

[Cameron via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: clayborg, zturner, jingham.
cameron314 added subscribers: mamai, lldb-commits.
cameron314 set the repository for this revision to rL LLVM.

When stopping the private state thread, there was a race condition between the 
time the thread exits (resetting the HostThread object) and the time a Join was 
attempted, especially in the case of a timeout.

The previous workaround of copying the HostThread object is not enough, since 
on a Reset the internal thread stuff gets nulled out regardless of which 
HostThread object actually has Reset called on it, resulting in an attempt to 
dereference a null pointer on the subsequent call to Join from the copy as well.

In our environment (custom target), we were hitting this every time we stopped 
debugging.

Repository:
  rL LLVM

http://reviews.llvm.org/D19122

Files:
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, 
cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
 TimeValue timeout_time;
 bool timed_out;
@@ -4134,7 +4131,7 @@
 {
 if (timed_out)
 {
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
 if (log)
 log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
 }
@@ -4145,7 +4142,7 @@
 }
 
 thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
 m_private_state_thread.Reset();
 }
 }
@@ -4449,7 +4446,6 @@
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
 m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
 return NULL;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: tools/lldb-mi/MIUtilFileStd.cpp:234-241
@@ -223,11 +233,10 @@
 
-FILE *pTmp = nullptr;
-pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+FILE *pTmp = lldb_private::FileSystem::Fopen(vFileNamePath.c_str(), "wb");
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
 return true;
 }
 
 return false;
 }

Seems this breaks the LLDB build on OS X. Sean has reverted this part, but it 
should be using the `#ifdef` that was there in the earlier version of the patch:

#ifdef _WIN32
std::wstring path;
if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
return false;
pTmp = ::_wfopen(path.c_str(), L"wb");
#else
pTmp = ::fopen(vFileNamePath.c_str(), "wb");
#endif


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 closed this revision.
cameron314 added a comment.

Closing now that the patch has been committed by 
http://reviews.llvm.org/rL264074.
(Not sure why it didn't auto-link.)


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Excellent, thank you!
Fortunately, almost all the other changes I've made to llvm/clang/lldb locally 
are fairly small patches.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 51231.
cameron314 added a comment.

Here's the latest patch, now with 100% more autoformatting and a centralized 
`FileSystem::Stat` function.
I didn't rebase, but it should still apply cleanly.


http://reviews.llvm.org/D17107

Files:
  cmake/modules/LLDBConfig.cmake
  include/lldb/Host/FileSystem.h
  include/lldb/Host/posix/HostInfoPosix.h
  include/lldb/Host/windows/HostInfoWindows.h
  packages/Python/lldbsuite/test/dotest.py
  source/Commands/CommandCompletions.cpp
  source/Core/ConnectionSharedMemory.cpp
  source/Core/Disassembler.cpp
  source/Host/common/File.cpp
  source/Host/common/FileSpec.cpp
  source/Host/posix/FileSystem.cpp
  source/Host/posix/HostInfoPosix.cpp
  source/Host/windows/ConnectionGenericFileWindows.cpp
  source/Host/windows/FileSystem.cpp
  source/Host/windows/Host.cpp
  source/Host/windows/HostInfoWindows.cpp
  source/Host/windows/HostProcessWindows.cpp
  source/Host/windows/PipeWindows.cpp
  source/Host/windows/ProcessLauncherWindows.cpp
  source/Host/windows/Windows.cpp
  source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  source/Target/ProcessLaunchInfo.cpp
  tools/driver/Driver.cpp
  tools/driver/Platform.h
  tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  tools/lldb-mi/MIUtilFileStd.cpp
  tools/lldb-mi/Platform.h

Index: tools/lldb-mi/Platform.h
===
--- tools/lldb-mi/Platform.h
+++ tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: tools/lldb-mi/MIUtilFileStd.cpp
===
--- tools/lldb-mi/MIUtilFileStd.cpp
+++ tools/lldb-mi/MIUtilFileStd.cpp
@@ -14,8 +14,11 @@
 #include 
 
 // In-house headers:
-#include "MIUtilFileStd.h"
 #include "MICmnResources.h"
+#include "MIUtilFileStd.h"
+#include "lldb/Host/FileSystem.h"
+
+#include "llvm/Support/ConvertUTF.h"
 
 //++ 
 // Details: CMIUtilFileStd constructor.
@@ -82,7 +85,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -221,8 +231,7 @@
 if (vFileNamePath.empty())
 return false;
 
-FILE *pTmp = nullptr;
-pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+FILE *pTmp = lldb_private::FileSystem::Fopen(vFileNamePath.c_str(), "wb");
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -18,15 +18,16 @@
 #include "lldb/API/SBBreakpointLocation.h"
 
 // In-house headers:
+#include "MICmdData.h"
 #include "MICmnLLDBDebugSessionInfo.h"
 #include "MICmnLLDBDebugger.h"
-#include "MICmnResources.h"
+#include "MICmnLLDBUtilSBValue.h"
 #include "MICmnMIResultRecord.h"
 #include "MICmnMIValueConst.h"
 #include "MICmnMIValueList.h"
 #include "MICmnMIValueTuple.h"
-#include "MICmdData.h"
-#include "MICmnLLDBUtilSBValue.h"
+#include "MICmnResources.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: tools/driver/Platform.h
===
--- tools/driver/Platform.h
+++ tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: tools/driver/Driver.cpp
===
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -27,7 +27,6 @@
 
 #include 
 
-#include 
 #include "lldb/API/SBBreakpoint.h"
 #include 

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I'll run clang-format and submit a new patch. Thanks for fixing that duplicate 
symbol with the signal stuff!



Comment at: source/Host/common/FileSpec.cpp:94-115
@@ -93,2 +93,24 @@
+{
+#ifdef _WIN32
+std::wstring wresolved_path;
+if (!llvm::ConvertUTF8toWide(resolved_path, wresolved_path))
+return false;
+int stat_result;
+#ifdef _USE_32BIT_TIME_T
+struct _stat32 file_stats;
+stat_result = ::_wstat32(wresolved_path.c_str(), &file_stats);
+#else
+struct _stat64i32 file_stats;
+stat_result = ::_wstat64i32(wresolved_path.c_str(), &file_stats);
+#endif
+if (stat_result == 0)
+{
+static_assert(sizeof(struct stat) == sizeof(file_stats), "stat and 
_stat32/_stat64i32 must have the same layout");
+*stats_ptr = *reinterpret_cast(&file_stats);
+}
+return stat_result == 0;
+#else
 return ::stat (resolved_path, stats_ptr) == 0;
+#endif
+}
 return false;

zturner wrote:
> Here we are doing some stuff with `_USE_32BIT_TIME_T`, but we reproduce 
> almost this exact same logic in `File::GetPermissions()` except without the 
> special case for `_USE_32BIT_TIME_T`.  I think we should introduce a function 
> called `FileSystem::Stat()` in very much the same way that you've done 
> `FileSystem::Fopen()`.  And then we put the implementation there so that we 
> can have it in one place and make sure it's always going to be consistent and 
> correct.  
Yeah, I agree this should really go in `FileSystem`.


Comment at: tools/lldb-mi/MIUtilFileStd.cpp:82-95
@@ -79,9 +81,16 @@
 
 #if !defined(_MSC_VER)
 // Open with 'write' and 'binary' mode
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 

zturner wrote:
> Should we be using `FileSystem::Fopen()` here?
Since this is MSVC specific code already, I think it's OK. The `_SH_DENYWR` 
seems important here.


Comment at: tools/lldb-mi/MIUtilFileStd.cpp:234-241
@@ -224,3 +233,10 @@
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)

zturner wrote:
> Same here.  `FileSystem::Fopen()`
Absolutely. I missed this one.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I think we're in different time zones -- I'm heading home for the day, but I'll 
look back in the morning to see if there's anything else that needs addressing.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 50979.

http://reviews.llvm.org/D17107

Files:
  cmake/modules/LLDBConfig.cmake
  include/lldb/Host/FileSystem.h
  include/lldb/Host/posix/HostInfoPosix.h
  include/lldb/Host/windows/HostInfoWindows.h
  packages/Python/lldbsuite/test/dotest.py
  source/Commands/CommandCompletions.cpp
  source/Core/ConnectionSharedMemory.cpp
  source/Core/Disassembler.cpp
  source/Host/common/File.cpp
  source/Host/common/FileSpec.cpp
  source/Host/posix/FileSystem.cpp
  source/Host/posix/HostInfoPosix.cpp
  source/Host/windows/ConnectionGenericFileWindows.cpp
  source/Host/windows/FileSystem.cpp
  source/Host/windows/Host.cpp
  source/Host/windows/HostInfoWindows.cpp
  source/Host/windows/HostProcessWindows.cpp
  source/Host/windows/PipeWindows.cpp
  source/Host/windows/ProcessLauncherWindows.cpp
  source/Host/windows/Windows.cpp
  source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  source/Target/ProcessLaunchInfo.cpp
  tools/driver/Driver.cpp
  tools/driver/Platform.h
  tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  tools/lldb-mi/MIUtilFileStd.cpp
  tools/lldb-mi/Platform.h

Index: tools/lldb-mi/Platform.h
===
--- tools/lldb-mi/Platform.h
+++ tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: tools/lldb-mi/MIUtilFileStd.cpp
===
--- tools/lldb-mi/MIUtilFileStd.cpp
+++ tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: tools/driver/Platform.h
===
--- tools/driver/Platform.h
+++ tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: tools/driver/Driver.cpp
===
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBProcess.h"
+#include "llvm/Support/ConvertUTF.h"
 
 #if !defined(__APPLE__)
 #include "llvm/Support/DataTypes.h"
@@ -1298,14 +1299,30 @@
 }
 
 int
-main (int argc, char const *argv[], const char *envp[])
+#ifdef WIN32
+wmain(int argc, wchar_t const *wargv[])
+#else
+main (int argc, char const *argv[])
+#endif
 {
 #ifdef _MSC_VER
 	// disable buffering on windows
 	setvbuf(stdout, NULL, _IONBF, 0);
 	setvbuf(stdin , NULL, _IONBF, 0);
 #endif
 
+#ifdef _WIN32
+// Convert wide arguments to UTF-8
+std::vector argvStrings(

Re: [Lldb-commits] [PATCH] D18287: Don't try to redefine the signal() function on Windows

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: tools/driver/Driver.cpp:1314
@@ -1313,1 +1313,3 @@
+signal(SIGINT, sigint_handler);
+#ifndef _MSC_VER
 signal (SIGPIPE, SIG_IGN);

zturner wrote:
> I think `_MSC_VER` is the right check, because the builtin `signal` 
> implementation is something that is part of Visual Studio.  If you were using 
> Cygwin I imagine some of those constants might be present (although tbh, I 
> don't ever use Cygwin and I don't know if anyone else does either, so someone 
> would need to confirm).
> 
> `_MSC_VER` is also defined for clang-cl, so this check would catch MSVC and 
> clang-cl
I was thinking more of mingw-w64, which tries very hard to have a compatible 
set of headers as those defined in the Windows SDK (and does not define 
`_MSC_VER`). But many, many other places in LLDB already check `_MSC_VER` 
anyway.


Comment at: tools/driver/Platform.cpp:93-97
@@ -92,7 @@
-{
-case ( SIGINT ):
-{
-_ctrlHandler = sigFunc;
-SetConsoleCtrlHandler( CtrlHandler, TRUE );
-}
-break;

zturner wrote:
> Yes, because now `MIDriverMain.cpp` should end up calling the builtin version 
> of `signal()`, which also calls `SetConsoleCtrlHandler`.  As far as I can 
> tell there's no actual difference (technically, the builtin version is a 
> strict superset of this version, so if anything it should be better)
Great!


Comment at: tools/driver/Platform.h:40
@@ -39,14 +39,3 @@
 
-
-// signal handler function pointer type
-typedef void(*sighandler_t)(int);
-
-// signal.h
-#define SIGINT 2
-// default handler
-#define SIG_DFL ( (sighandler_t) -1 )
-// ignored
-#define SIG_IGN ( (sighandler_t) -2 )
-
 // signal.h
 #define SIGPIPE  13

amccarth wrote:
> Now that we are including , won't the following defines create 
> duplicate definition warnings for the non-Windows platforms?
It's only included on Windows, though -- unless some other TU includes both 
this header and `signal.h`?


http://reviews.llvm.org/D18287



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Since it works without my patch, you're probably right about it being related 
to the UNICODE define. All the other changes are completely removed from that 
part of the code. But I still don't see how it could affect that.

Here's the script I use to run cmake:

  setlocal 
  set 
PATH=%ProgramFiles%\Ninja;%ProgramFiles(x86)%\CMake\bin;%SYSTEMROOT%\system32;%SYSTEMROOT%;%SWIGDIR%
 
  call "%VS140COMNTOOLS%\vsvars32.bat" 
  mkdir VS2015Debug 
  cd VS2015Debug 
  cmake -G "Ninja" -DLLVM_BUILD_EXAMPLES:BOOL=false -DPYTHON_HOME="C:\Python35" 
-DLLDB_DISABLE_PYTHON:BOOL=false -DLLDB_TEST_COMPILER=%cd%\bin\clang.exe 
-DCMAKE_BUILD_TYPE=Debug .. 
  cd .. 
  mkdir VS2015Release 
  cd VS2015Release 
  cmake -G "Ninja" -DLLVM_BUILD_EXAMPLES:BOOL=false -DPYTHON_HOME="C:\Python35" 
-DLLDB_DISABLE_PYTHON:BOOL=false -DLLDB_TEST_COMPILER=%cd%\bin\clang.exe 
-DCMAKE_BUILD_TYPE=Release .. 
  cd .. 
  endlocal

This is with VS2015 update 1. And it's already the x86 version, the patch has 
diverged significantly from what we have locally (which is based on a snapshot 
from several weeks ago) so I've been using the tip as-is :-)

I'm trying a fresh Debug build from scratch (deleting and re-creating the cmake 
build folder). I'll report back when it's done.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Hmm, that doesn't seem good. Locally it links for me (I get compile time 
warnings about the signal stuff, though).
It looks like the WinSDK signal.h is being pulled in despite `_INC_SIGNAL` 
being defined (note that there's no include guard in that header -- it uses 
`#pragma once` instead). And the signal handler callback has a different return 
type. I don't see anything related to Unicode.

Here's the tail of my build output for lldb-mi.exe. I'm using VS2015 on Windows 
7 x64, compiling in release with ninja:

  [77/78] 13.653s Building CXX object 
tools\lldb\tools\lldb-mi\CMakeFiles\lldb-mi.dir\Platform.cpp.obj
  d:\dev\llvm\tools\lldb\tools\lldb-mi\Platform.h(84): warning C4005: 
'SIG_DFL': macro redefinition
  C:\Program Files (x86)\Windows 
Kits\10\include\10.0.10240.0\ucrt\signal.h(35): note: see previous definition 
of 'SIG_DFL'
  d:\dev\llvm\tools\lldb\tools\lldb-mi\Platform.h(85): warning C4005: 
'SIG_IGN': macro redefinition
  C:\Program Files (x86)\Windows 
Kits\10\include\10.0.10240.0\ucrt\signal.h(36): note: see previous definition 
of 'SIG_IGN'
  d:\dev\llvm\tools\lldb\tools\lldb-mi\Platform.h(87): warning C4273: 'signal': 
inconsistent dll linkage
  C:\Program Files (x86)\Windows 
Kits\10\include\10.0.10240.0\ucrt\signal.h(57): note: see previous definition 
of 'signal'
  
  [78/78] 14.332s Linking CXX executable bin\lldb-mi.exe
 Creating library lib\lldb-mi.lib and object lib\lldb-mi.exp


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Ah. Sorry. I have the same source layout as you locally, but I changed the 
paths in the patch to match the layout of Diffusion 
(http://reviews.llvm.org/diffusion/L/). I'll redo the patch with relative paths 
from lldb...


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 50976.
cameron314 added a comment.

Here we go! Rebased to tip (http://reviews.llvm.org/rL263735).


http://reviews.llvm.org/D17107

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/posix/FileSystem.cpp
  lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: lldb/trunk/tools/driver/Driver.cpp
===
--- lldb/trunk/tools/driver/Driver.cpp
+++ lldb/trunk/tools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #include "lldb/API/SBTarget.

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Aha, I get the same error now:

  fatal error LNK1169: one or more multiply defined symbols found

I'm looking into it!


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

@zturner: Let me know when you're ready for this patch and I'll rebase it 
again, since there's been quite a few more commits.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D18287: Don't try to redefine the signal() function on Windows

[Cameron via lldb-commits]

cameron314 added a comment.

Cool! I got pulled into something else at work and didn't have time to 
investigate the linker error that this led to, sorry. But the patch looks good 
to me (not that I know LLDB very well).



Comment at: tools/driver/Driver.cpp:1314
@@ -1313,1 +1313,3 @@
+signal(SIGINT, sigint_handler);
+#ifndef _MSC_VER
 signal (SIGPIPE, SIG_IGN);

Might be slightly more portable to use `_WIN32` to detect the OS platform 
instead of detecting the compiler?


Comment at: tools/driver/Platform.cpp:93-97
@@ -92,7 @@
-{
-case ( SIGINT ):
-{
-_ctrlHandler = sigFunc;
-SetConsoleCtrlHandler( CtrlHandler, TRUE );
-}
-break;

It looks like MIDriverMain.cpp was using this. Will it continue to work without 
the console control handler?


Comment at: tools/driver/Platform.h:15
@@ -14,4 +14,3 @@
 
 // this will stop signal.h being included
 #include "lldb/Host/HostGetOpt.h"

This comment is no longer relevant.


http://reviews.llvm.org/D18287



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

No worries. I removed the codepage stuff when I did the last rebase. Another 
one coming up shortly!


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 50449.
cameron314 added a comment.

... and had a slash too many at the start of the path prefixes. Sorry for the 
spam.


http://reviews.llvm.org/D17107

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/posix/FileSystem.cpp
  lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: lldb/trunk/tools/driver/Driver.cpp
===
--- lldb/trunk/tools/driver/Driver.cpp
+++ lldb/trunk/tools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #inclu

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 50448.
cameron314 added a comment.

Oops, was missing a slash in the dst-prefix of the previous patch update.


http://reviews.llvm.org/D17107

Files:
  /lldb/trunk/cmake/modules/LLDBConfig.cmake
  /lldb/trunk/include/lldb/Host/FileSystem.h
  /lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  /lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  /lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  /lldb/trunk/source/Commands/CommandCompletions.cpp
  /lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  /lldb/trunk/source/Core/Disassembler.cpp
  /lldb/trunk/source/Host/common/File.cpp
  /lldb/trunk/source/Host/common/FileSpec.cpp
  /lldb/trunk/source/Host/posix/FileSystem.cpp
  /lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  /lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  /lldb/trunk/source/Host/windows/FileSystem.cpp
  /lldb/trunk/source/Host/windows/Host.cpp
  /lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  /lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  /lldb/trunk/source/Host/windows/PipeWindows.cpp
  /lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  /lldb/trunk/source/Host/windows/Windows.cpp
  /lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  /lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  /lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  /lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  /lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  /lldb/trunk/tools/driver/Driver.cpp
  /lldb/trunk/tools/driver/Platform.h
  /lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  /lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  /lldb/trunk/tools/lldb-mi/Platform.h

Index: /lldb/trunk/tools/lldb-mi/Platform.h
===
--- /dev/null
+++ /lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: /lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- /dev/null
+++ /lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: /lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- /dev/null
+++ /lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: /lldb/trunk/tools/driver/Platform.h
===
--- /dev/null
+++ /lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: /lldb/trunk/tools/driver/Driver.cpp
===
--- /dev/null
+++ /lldb/trunk/tools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBProcess.h"
+#include "llvm/Support/ConvertUTF.

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 50447.
cameron314 added a comment.

Here we go! http://reviews.llvm.org/D17549 has been committed, so I've rebased 
this patch onto that latest version (r263233).


http://reviews.llvm.org/D17107

Files:
  /lldb/trunkcmake/modules/LLDBConfig.cmake
  /lldb/trunkinclude/lldb/Host/FileSystem.h
  /lldb/trunkinclude/lldb/Host/posix/HostInfoPosix.h
  /lldb/trunkinclude/lldb/Host/windows/HostInfoWindows.h
  /lldb/trunkpackages/Python/lldbsuite/test/dotest.py
  /lldb/trunksource/Commands/CommandCompletions.cpp
  /lldb/trunksource/Core/ConnectionSharedMemory.cpp
  /lldb/trunksource/Core/Disassembler.cpp
  /lldb/trunksource/Host/common/File.cpp
  /lldb/trunksource/Host/common/FileSpec.cpp
  /lldb/trunksource/Host/posix/FileSystem.cpp
  /lldb/trunksource/Host/posix/HostInfoPosix.cpp
  /lldb/trunksource/Host/windows/ConnectionGenericFileWindows.cpp
  /lldb/trunksource/Host/windows/FileSystem.cpp
  /lldb/trunksource/Host/windows/Host.cpp
  /lldb/trunksource/Host/windows/HostInfoWindows.cpp
  /lldb/trunksource/Host/windows/HostProcessWindows.cpp
  /lldb/trunksource/Host/windows/PipeWindows.cpp
  /lldb/trunksource/Host/windows/ProcessLauncherWindows.cpp
  /lldb/trunksource/Host/windows/Windows.cpp
  /lldb/trunksource/Plugins/Process/Windows/Live/DebuggerThread.cpp
  /lldb/trunksource/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  /lldb/trunksource/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  /lldb/trunksource/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  /lldb/trunksource/Target/ProcessLaunchInfo.cpp
  /lldb/trunktools/driver/Driver.cpp
  /lldb/trunktools/driver/Platform.h
  /lldb/trunktools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  /lldb/trunktools/lldb-mi/MIUtilFileStd.cpp
  /lldb/trunktools/lldb-mi/Platform.h

Index: /lldb/trunktools/lldb-mi/Platform.h
===
--- /dev/null
+++ /lldb/trunktools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: /lldb/trunktools/lldb-mi/MIUtilFileStd.cpp
===
--- /dev/null
+++ /lldb/trunktools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: /lldb/trunktools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- /dev/null
+++ /lldb/trunktools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: /lldb/trunktools/driver/Platform.h
===
--- /dev/null
+++ /lldb/trunktools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: /lldb/trunktools/driver/Driver.cpp
===
--- /dev/null
+++ /lldb/trunktools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBProcess.h"
+#include "llvm/Suppor

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Yes, if lldb.exe crashes or otherwise exits without returning to main, then the 
codepage will stay set in the current console. The latter can be fixed with a 
static object destructor, but not the former. I don't think this would be a 
practical problem, since very few console applications depend on the codepage 
in the first place (those that do tend to set it manually when they start). The 
setting isn't permanent, either, it's only for the current console window until 
it closes (or is reset by somebody else). And it doesn't affect the bytes 
input/output by any programs, only the way the console interprets those bytes. 
I'd say the impact of changing the codepage without changing it back is very 
low.

That part of the patch is not quite as important -- without it UTF-8 I/O is 
broken, but it's already broken anyway unless the user has taken explicit steps 
to reconfigure Window's console system to support Unicode character display 
(which requires fiddling in the registry and rebooting). No one I know has done 
this. I can remove it from the patch if you think it's too risky.

We're using both liblldb and lldb.exe, though mostly liblldb. No Python 
bindings as of now.

The patch for LLVM has been accepted, I'm waiting for someone to commit it when 
they have a moment. I'll rebase this one on the tip again afterwards.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 49986.
cameron314 added a comment.

Sorry for the delay, I wanted to make sure LLDB compiled without warnings after 
I rebased the patch. It applies cleanly on the tip as of this writing, and is 
based on http://reviews.llvm.org/rL262819.


http://reviews.llvm.org/D17107

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/posix/FileSystem.cpp
  lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#ifdef _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: lldb/trunk/tools/driver/Driver.cpp
=

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+}
+::_wsopen_s(&m_descriptor, wpath.c_str(), oflag, _SH_DENYWR, mode);
+#else

zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Any particular reason you're using `_SH_DENYWR` instead of `_SH_DENYNO`?  
> > > No matter what we do we will always have subtle differences in semantics, 
> > > but `_SH_DENYNO` is closer to posix semantics.
> > No particular reason other than I find `_SH_DENYWR` a reasonable default. 
> > It generally doesn't hurt to let others read the file while it's being 
> > written, and that's sometimes crucial (e.g. for log files which are written 
> > to incrementally and only closed at the very end). I can change it if you 
> > prefer.
> I like deny write in principle as well, but I kind of do prefer to change it 
> if for no other reason than it matches the semantics on Posix more closely.  
> We've had sharing access denied errors in the past on Windows that we traced 
> back to sharing violations, and it's easier if all platforms just use the 
> same semantics.
OK, I'll change it for the next patch.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Absolutely, I'm rebasing now. This is unfortunately the type of patch that rots 
really quickly ;-)

I did run `check-lldb` locally at one point, with no major differences before 
and after the patch. But I recently discovered the version of Python I'd 
compiled had accidentally been compiled with VS2008, and since the tip's 
changed anyway, it's absolutely worth running them again. This was the day 
before the instructions to use VS21015 with Python 3 were posted, haha.



Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+}
+::_wsopen_s(&m_descriptor, wpath.c_str(), oflag, _SH_DENYWR, mode);
+#else

zturner wrote:
> Any particular reason you're using `_SH_DENYWR` instead of `_SH_DENYNO`?  No 
> matter what we do we will always have subtle differences in semantics, but 
> `_SH_DENYNO` is closer to posix semantics.
No particular reason other than I find `_SH_DENYWR` a reasonable default. It 
generally doesn't hurt to let others read the file while it's being written, 
and that's sometimes crucial (e.g. for log files which are written to 
incrementally and only closed at the very end). I can change it if you prefer.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

No worries, thanks for taking a look!


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I don't want to be pushy, but is there a chance of this patch being accepted 
soon?
It does depend on http://reviews.llvm.org/D17549 in LLVM which is also still 
pending, but that one's at least very a simple patch adding the support 
functions that this one uses.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 updated this revision to Diff 49185.

http://reviews.llvm.org/D17107

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/posix/FileSystem.cpp
  lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#if _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STDIN_FILENO 0
Index: lldb/trunk/tools/driver/Driver.cpp
===
--- lldb/trunk/tools/driver/Driver.cpp
+++ lldb/trunk/tools/driver/Driver.cpp
@@ -42,6 +42,7 @@
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBProcess.h"
+#include "llvm/Support/Conv

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:242
@@ -221,1 +241,3 @@
+path.push_back(0);
+path.pop_back();
 }

amccarth wrote:
> I recognize that you're just repeating the pattern from above, but ...
> 
> This seems whacky and dangerous.  It appears the attempt is to put a null 
> terminator on the end, but not count it in the length of the vector.  And I 
> guess that we know it's safe here because path is an llvm::SmallVector, so 
> the implementation details are known.  But, ugh.  If `path` were ever changed 
> to std::vector, we'd no longer have assurance that the terminator remains 
> after the pop.
I totally agree. Took me a few minutes before I figured out what it was doing 
the first time I saw it :-) This is also done in the existing 
`convertUTF8ToUTF16String` wrapper too.

All the implementations of `std::vector` that I know of will work with this, 
though as you say it's not guaranteed by the standard.

Looking more closely, I think in this case it was only done for the call to 
`stat`, which I've removed. I had added it here too in case the caller relied 
on this null trick, but I don't think it's necessary in either place anymore. 
I'll remove it.


Comment at: lldb/trunk/source/Host/windows/FileSystem.cpp:231
@@ -191,3 +230,3 @@
 
-char buf[PATH_MAX];
+wchar_t buf[PATH_MAX + 1];
 // Subtract 1 from the path length since this function does not add a null 
terminator.

amccarth wrote:
> I agree with Zach that a dynamic solution here is better.  It's already icky 
> that we have a 32KB stack buffer here.  Turning it into a 64KB +1B stack 
> buffer seem egregious.
Hah, yes. I'll replace this with a vector to start with.


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:618
@@ -616,3 +617,3 @@
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = 
rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));

Unrelated to this patch, but can this method be called from several threads at 
once?


http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 49066.
cameron314 added a comment.

Here's a new version of the patch which takes into account most of the feedback 
so far (less `#ifdefs`, etc.). It depends on my pending patch in LLVM 
(http://reviews.llvm.org/D17549) that introduces a few more helper wrappers for 
UTF conversion.


http://reviews.llvm.org/D17107

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/include/lldb/Host/FileSystem.h
  lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
  lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/posix/FileSystem.cpp
  lldb/trunk/source/Host/posix/HostInfoPosix.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,8 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +84,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+std::wstring path;
+if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +231,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#if _WIN32
+std::wstring path;
+if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STD

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:1179
@@ +1178,3 @@
+
+char child_path[PATH_MAX];
+const int child_path_len = ::snprintf (child_path, 
sizeof(child_path), "%s\\%s", dir_path, fileName.c_str());

amccarth wrote:
> cameron314 wrote:
> > amccarth wrote:
> > > cameron314 wrote:
> > > > amccarth wrote:
> > > > > MAX_PATH in Windows.  (And someday, we should get rid of these fixed 
> > > > > buffers so we can use UNC paths.)
> > > > I absolutely agree re the fixed buffers. I'll try to make this change 
> > > > in all of the functions that can allocate dynamic memory.
> > > > 
> > > > `MAX_PATH` is not `PATH_MAX`, though -- `MAX_PATH` is a Win32 constant 
> > > > of 260, whereas `PATH_MAX` is an LLDB constant of 32K. (Actually 
> > > > PATH_MAX is defined in multiple places and was either that or MAX_PATH 
> > > > depending on which header was included, but mostly it was 32K so I 
> > > > changed them all to at least be consistently 32K.)
> > > Ah, I see.  I work in another code base where PATH_MAX is synonymous with 
> > > MAX_PATH, so I was confused.
> > > 
> > > Buffers of 32K characters on the stack seem like a bad idea.  We need a 
> > > vector or string or some other container that puts the bulk of the data 
> > > somewhere other than the stack.
> > Right. There's currently ~110 places in LLDB that allocate buffers of size 
> > `PATH_MAX` on the stack. Nearly all of those predate my patch, it seems.
> Right, but this is a new case because you changed it from MAX_PATH (260) to 
> PATH_MAX (32K).  I'd rather not introduce more instances like this.
Good point, I'll fix it up :-)


Repository:
  rL LLVM

http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/source/Core/Disassembler.cpp:881
@@ -878,3 +880,3 @@
 }
-
-FILE *test_file = fopen (file_name, "r");
+FILE *test_file = nullptr;
+#if _WIN32

zturner wrote:
> cameron314 wrote:
> > cameron314 wrote:
> > > zturner wrote:
> > > > Would LLDB's `File` class work here instead of using raw APIs?  I see 
> > > > you fixed the code in `lldb_private::File`, so perhaps simply using 
> > > > that class here instead of a `FILE*` would allow you to reuse the fix 
> > > > from that class.
> > > No idea. Probably :-) I'll look into it, and make this change if it's 
> > > trivial.
> > I don't trust myself to make this change without introducing a bug. But 
> > I'll get rid of the `#ifdef`.
> One possibility is to revert this portion of the patch (along with any other 
> changes that you don't feel comfortable making) back to their original state. 
>  This will still leave certain parts of unicode functionality broken, but it 
> would reduce the surface area of "things that might be wrong with the patch" 
> and give people a chance to make sure nothing else is seriously broken.  
> Then, we can do these other "more difficult" locations as a followup patch.  
> The nice thing about that is that if it does end up breaking something, and 
> we have to revert it, it doesn't revert 100% of the work towards getting 
> Unicode working, but only a couple of points.
This makes sense. I'm almost done with the next version of the patch, we'll see 
what should make the cut at that point.


Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:1179
@@ +1178,3 @@
+
+char child_path[PATH_MAX];
+const int child_path_len = ::snprintf (child_path, 
sizeof(child_path), "%s\\%s", dir_path, fileName.c_str());

amccarth wrote:
> cameron314 wrote:
> > amccarth wrote:
> > > MAX_PATH in Windows.  (And someday, we should get rid of these fixed 
> > > buffers so we can use UNC paths.)
> > I absolutely agree re the fixed buffers. I'll try to make this change in 
> > all of the functions that can allocate dynamic memory.
> > 
> > `MAX_PATH` is not `PATH_MAX`, though -- `MAX_PATH` is a Win32 constant of 
> > 260, whereas `PATH_MAX` is an LLDB constant of 32K. (Actually PATH_MAX is 
> > defined in multiple places and was either that or MAX_PATH depending on 
> > which header was included, but mostly it was 32K so I changed them all to 
> > at least be consistently 32K.)
> Ah, I see.  I work in another code base where PATH_MAX is synonymous with 
> MAX_PATH, so I was confused.
> 
> Buffers of 32K characters on the stack seem like a bad idea.  We need a 
> vector or string or some other container that puts the bulk of the data 
> somewhere other than the stack.
Right. There's currently ~110 places in LLDB that allocate buffers of size 
`PATH_MAX` on the stack. Nearly all of those predate my patch, it seems.


Comment at: lldb/trunk/source/Host/windows/Windows.cpp:210
@@ +209,3 @@
+wchar_t wpath[PATH_MAX];
+if (wchar_t* wresult = _wgetcwd(wpath, PATH_MAX))
+{

zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Why can't we just convert to the result of `_wgetcwd` directly to UTF8?
> > Not sure I understand. That's what this code does? There's a tad of extra 
> > logic needed because the `path` passed in is allowed to be `NULL`, 
> > indicating we should allocate a buffer for the caller.
> Ahh that extra logic is what I was referring to.  I wasn't aware of those 
> semantics of `getcwd`.  Can you add a comment explaining that, since it's not 
> obvious from looking at the code.
Haha, sure. I wasn't aware either, but it blew up at runtime since it turns out 
LLDB actually makes use of this when booting with a relative path.


Comment at: 
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1175
@@ -1166,1 +1174,3 @@
+fp = _wfopen((const wchar_t *)wpath.data(), (const wchar_t *)wmode.data());
+#else
 fp = fopen(path, mode);

zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Here's another example of an #ifdef in common code that we will need to 
> > > do something about.  Is there a constructor of the `File` class that 
> > > takes a path and a mode?
> > There is, but the mode is an enum and not a string. I'll get rid of the 
> > `#ifdef`, though.
> We have a function somewhere that converts a mode string to the enum.  I 
> think it's in `File.h` or `File.cpp`.  Let me know if you can't find it.
You know, it turns out there's one right in this very file. Thanks!


Repository:
  rL LLVM

http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added inline comments.


Comment at: lldb/trunk/tools/driver/Driver.cpp:1289
@@ +1288,3 @@
+// Indicate that all our output is in UTF-8
+SetConsoleCP(CP_UTF8);
+#endif

zturner wrote:
> Is this going to affect the state of the console even after quitting LLDB?
Hmm, turns out it does. I'll add code to revert it at the end.


Repository:
  rL LLVM

http://reviews.llvm.org/D17107



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

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I've addressed more feedback. I'm working on a second version of the patch with 
a lot less `#ifdefs` and nicer wrappers (and better error handling where 
possible).



Comment at: lldb/trunk/source/Commands/CommandCompletions.cpp:171
@@ -168,1 +170,3 @@
 {
+#ifdef _WIN32
+llvm::SmallVector wpartial_name_copy;

zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > I'm not fond of this approach of sprinkling #ifdefs around in every 
> > > location where we call into a file system API.  We have 
> > > `lldb/Host/FileSystem`, probably some of these methods should be moved 
> > > over there, and we should provide a windows implementation and a 
> > > non-windows implementation.  This way in places like this, you simply 
> > > write:
> > > 
> > > isa_directory = FileSystem::GetPermissions(partial_name_copy) & 
> > > FileSystem::eDirectory);
> > > 
> > > and all this ugliness is hidden.
> > I agree. Again, my goal was not to refactor (I don't know this codebase 
> > very well at all and can't test it in much depth), but to fix existing 
> > code. It happens that `stat` is particularly awkward on Win32, so yes, it 
> > should probably be wrapped instead (or better yet replaced with 
> > `GetFileAttributes()`). But this is outside the scope of the patch.
> I know your intent was to just fix existing code, but when those fixes make 
> the code harder to maintain or do things the wrong way, then I think it's 
> worth asking whether the scope should expand.  Introducing ifdefs in common 
> code is not a direction we want to move in unless it's absolutely necessary, 
> so somehow we have to address that before this can go in, even if it means 
> expanding the scope of the patch slightly.
I understand. I'll try to refactor out the ifdefs.


Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:140
@@ +139,3 @@
+llvm::SmallVector wname;
+if (llvm::convertUTF8ToUTF16String(name, wname))
+{

zturner wrote:
> cameron314 wrote:
> > amccarth wrote:
> > > It's too bad there isn't an overload that gives the wide string as a 
> > > std::wstring.  Then we wouldn't need all the ugly C-style casts of 
> > > wname.data().
> > I didn't want to add/change the string conversion support libraries too 
> > much (there's already too many functions with overlapping functionality 
> > IMO), but on the whole this would indeed yield much nicer and simpler code. 
> > I'll make this change.
> It's going to be difficult to make this change.  Because that will require a 
> separate review on the LLVM side with different reviewers.  You can certainly 
> try though.
Hmm. Alright then. I'll put the wrappers in LLDB for now.


Comment at: lldb/trunk/source/Core/Disassembler.cpp:881
@@ -878,3 +880,3 @@
 }
-
-FILE *test_file = fopen (file_name, "r");
+FILE *test_file = nullptr;
+#if _WIN32

cameron314 wrote:
> zturner wrote:
> > Would LLDB's `File` class work here instead of using raw APIs?  I see you 
> > fixed the code in `lldb_private::File`, so perhaps simply using that class 
> > here instead of a `FILE*` would allow you to reuse the fix from that class.
> No idea. Probably :-) I'll look into it, and make this change if it's trivial.
I don't trust myself to make this change without introducing a bug. But I'll 
get rid of the `#ifdef`.


Comment at: lldb/trunk/source/Host/common/File.cpp:353
@@ +352,3 @@
+int stat_result;
+#ifdef _WIN32
+llvm::SmallVector wpath;

cameron314 wrote:
> zturner wrote:
> > Ahh, here's that method I was looking for earlier when I mentioend 
> > `FileSystem`.  So I guess you don't have to implement it, it's already 
> > here.  So you could just use this method earlier in the part that gets file 
> > permissions by manually calling stat.
> This method only returns the permissions -- the other one you're referring to 
> checks if a path is a directory.
...which turns out to exist here too. I call that instead of using stat now, 
much cleaner!


Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:108
@@ +107,3 @@
+if (stat_result == 0)
+*stats_ptr = *reinterpret_cast(&file_stats);
+return stat_result == 0;

zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > I don't think this line is correct.  The source and destination struct 
> > > types do not always have the same layout, so I think you may need to copy 
> > > the values out one by one.
> > It's probably better if I copy the fields for the sake of forwards 
> > compatibility, but I checked the definitions of these structures and they 
> > should be 100% compatible. I'll make the change, though, the 
> > `reinterpret_cast` is really ugly.
> Apparently the CRT uses a reinterpret_cast too.  As long as you put:
> 
> static_assert(sizeof(struct stat) == sizeof(st

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I've replied to most of your comments, thanks everyone. I'll make most of the 
suggested changes, but some of them have nothing to do with correct Unicode 
handling and are rather pre-existing design issues, and so are out of the scope 
of this patch.

@zturner: I agree that the conversion as it is now is ugly, but I'm not sure a 
`WinUtfString` would be the best way to clean it up, considering the 
unfortunate necessity of error handling (which is still context-dependent 90% 
of the time). I'm also slightly allergic to adding more string classes, 
especially platform-specific ones ;-) I'm more inclined to simply adopt a 
cleaner set of std::string<->std::wstring conversion function wrappers as 
suggested by @amccarth. I'll start with that and we'll see from there.



Comment at: cfe/trunk/tools/libclang/CIndexer.cpp:57
@@ -55,3 +56,3 @@
   MEMORY_BASIC_INFORMATION mbi;
-  char path[MAX_PATH];
+  wchar_t wpath[MAX_PATH];
   VirtualQuery((void *)(uintptr_t)clang_createTranslationUnit, &mbi,

zturner wrote:
> You'll probably need to submit this as a separate patch to cfe-commits, since 
> it goes in a separate repo.  You can then commit them independently.
Alright, thanks. I'll do that. Thought it would make sense to sneak it in here 
;-)


Comment at: lldb/trunk/source/Commands/CommandCompletions.cpp:171
@@ -168,1 +170,3 @@
 {
+#ifdef _WIN32
+llvm::SmallVector wpartial_name_copy;

zturner wrote:
> I'm not fond of this approach of sprinkling #ifdefs around in every location 
> where we call into a file system API.  We have `lldb/Host/FileSystem`, 
> probably some of these methods should be moved over there, and we should 
> provide a windows implementation and a non-windows implementation.  This way 
> in places like this, you simply write:
> 
> isa_directory = FileSystem::GetPermissions(partial_name_copy) & 
> FileSystem::eDirectory);
> 
> and all this ugliness is hidden.
I agree. Again, my goal was not to refactor (I don't know this codebase very 
well at all and can't test it in much depth), but to fix existing code. It 
happens that `stat` is particularly awkward on Win32, so yes, it should 
probably be wrapped instead (or better yet replaced with 
`GetFileAttributes()`). But this is outside the scope of the patch.


Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:138
@@ -134,10 +137,3 @@
 #ifdef _WIN32
-HANDLE handle;
-if (create) {
-handle = CreateFileMapping(
-INVALID_HANDLE_VALUE,
-NULL,
-PAGE_READWRITE,
-llvm::Hi_32(size),
-llvm::Lo_32(size),
-name);
+HANDLE handle = INVALID_HANDLE_VALUE;
+llvm::SmallVector wname;

zturner wrote:
> Same thing applies here, we should probably have a `Host/SharedMemory.h` that 
> exposes a `Create()` method.  Although, in this particular case the `#ifdef` 
> was already here so I can't entirely complain, but it would be nice to 
> improve the code as we go and get ifdefs like this out of generic code.
Again, I agree, but this is outside the scope of my patch.


Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:140
@@ +139,3 @@
+llvm::SmallVector wname;
+if (llvm::convertUTF8ToUTF16String(name, wname))
+{

amccarth wrote:
> It's too bad there isn't an overload that gives the wide string as a 
> std::wstring.  Then we wouldn't need all the ugly C-style casts of 
> wname.data().
I didn't want to add/change the string conversion support libraries too much 
(there's already too many functions with overlapping functionality IMO), but on 
the whole this would indeed yield much nicer and simpler code. I'll make this 
change.


Comment at: lldb/trunk/source/Core/Disassembler.cpp:881
@@ -878,3 +880,3 @@
 }
-
-FILE *test_file = fopen (file_name, "r");
+FILE *test_file = nullptr;
+#if _WIN32

zturner wrote:
> Would LLDB's `File` class work here instead of using raw APIs?  I see you 
> fixed the code in `lldb_private::File`, so perhaps simply using that class 
> here instead of a `FILE*` would allow you to reuse the fix from that class.
No idea. Probably :-) I'll look into it, and make this change if it's trivial.


Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+}
+m_descriptor = ::_wopen((const wchar_t *)wpath.data(), oflag, mode);
+#else

amccarth wrote:
> _wopen is officially deprecated.  How about _wsopen_s?
Sure. I'll make this change. `open` was deprecated too...


Comment at: lldb/trunk/source/Host/common/File.cpp:353
@@ +352,3 @@
+int stat_result;
+#ifdef _WIN32
+llvm::SmallVector wpath;

zturner wrote:
> Ahh, here's that method I was looking for earlier when I mentioend 
> `FileSystem`.  So I gue

Re: [Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

Thanks for all the feedback! I'll look into it in detail tomorrow.

Perhaps it would make more sense if I clarified where I'm coming from with this 
patch -- we use LLDB on Windows (with a custom backend) where I work, and 
wanted to be able to run a program with a non-ASCII path (and similarly place 
breakpoints in such files, etc.). So, I tried to make //minimally invasive// 
changes across the codebase in order to accomplish this, using the existing 
LLVM support functions as much as possible. This is why the changes are not 
entirely consistent as a whole -- I preferred consistency with the surrounding 
code (e.g. error handling and buffer allocation) over global consistency.


Repository:
  rL LLVM

http://reviews.llvm.org/D17107



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

[Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 created this revision.
cameron314 added a reviewer: zturner.
cameron314 added a subscriber: lldb-commits.
cameron314 set the repository for this revision to rL LLVM.

Various fixes for Win32 interop, particularly with regard to paths; paths 
containing non-ASCII characters now work properly (they are converted to and 
from UTF-8 internally).

Repository:
  rL LLVM

http://reviews.llvm.org/D17107

Files:
  cfe/trunk/tools/libclang/CIndexer.cpp
  lldb/trunk/cmake/modules/LLDBConfig.cmake
  lldb/trunk/source/Commands/CommandCompletions.cpp
  lldb/trunk/source/Core/ConnectionSharedMemory.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Host/common/File.cpp
  lldb/trunk/source/Host/common/FileSpec.cpp
  lldb/trunk/source/Host/windows/ConnectionGenericFileWindows.cpp
  lldb/trunk/source/Host/windows/FileSystem.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Host/windows/HostInfoWindows.cpp
  lldb/trunk/source/Host/windows/HostProcessWindows.cpp
  lldb/trunk/source/Host/windows/PipeWindows.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  lldb/trunk/source/Host/windows/Windows.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/DebuggerThread.cpp
  lldb/trunk/source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp
  lldb/trunk/source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp
  lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/driver/Platform.h
  lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
  lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
  lldb/trunk/tools/lldb-mi/Platform.h
  llvm/trunk/include/llvm/Support/ConvertUTF.h
  llvm/trunk/lib/Support/CommandLine.cpp
  llvm/trunk/lib/Support/ConvertUTFWrapper.cpp
  llvm/trunk/unittests/Support/ConvertUTFTest.cpp

Index: lldb/trunk/tools/lldb-mi/Platform.h
===
--- lldb/trunk/tools/lldb-mi/Platform.h
+++ lldb/trunk/tools/lldb-mi/Platform.h
@@ -60,7 +60,7 @@
 typedef long pid_t;
 
 #define STDIN_FILENO 0
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #define snprintf _snprintf
 
 extern int ioctl(int d, int request, ...);
Index: lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
===
--- lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
+++ lldb/trunk/tools/lldb-mi/MIUtilFileStd.cpp
@@ -17,6 +17,9 @@
 #include "MIUtilFileStd.h"
 #include "MICmnResources.h"
 
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/ConvertUTF.h"
+
 //++ 
 // Details: CMIUtilFileStd constructor.
 // Type:Method.
@@ -82,7 +85,14 @@
 m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
 // Open a file with exclusive write and shared read permissions
-m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+llvm::SmallVector path;
+if (llvm::convertUTF8ToUTF16String(vFileNamePath.c_str(), path))
+m_pFileHandle = ::_wfsopen((const wchar_t *)path.data(), L"wb", _SH_DENYWR);
+else
+{
+errno = EINVAL;
+m_pFileHandle = nullptr;
+}
 #endif // !defined( _MSC_VER )
 
 if (m_pFileHandle == nullptr)
@@ -222,7 +232,14 @@
 return false;
 
 FILE *pTmp = nullptr;
+#if _WIN32
+llvm::SmallVector path;
+if (!llvm::convertUTF8ToUTF16String(vFileNamePath.c_str(), path))
+return false;
+pTmp = ::_wfopen((const wchar_t *)path.data(), L"wb");
+#else
 pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
 if (pTmp != nullptr)
 {
 ::fclose(pTmp);
Index: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -27,6 +27,7 @@
 #include "MICmnMIValueTuple.h"
 #include "MICmdData.h"
 #include "MICmnLLDBUtilSBValue.h"
+#include "Platform.h"
 
 //++ 
 // Details: CMICmnLLDBDebugSessionInfo constructor.
@@ -614,7 +615,7 @@
 {
 lldb::SBFrame &rFrame = const_cast(vrFrame);
 
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
 const MIuint nBytes = rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0], sizeof(pBuffer));
 MIunused(nBytes);
 CMIUtilString strResolvedPath(&pBuffer[0]);
Index: lldb/trunk/tools/driver/Platform.h
===
--- lldb/trunk/tools/driver/Platform.h
+++ lldb/trunk/tools/driver/Platform.h
@@ -81,7 +81,7 @@
 typedef long pid_t;
 #define snprintf _snprintf
 extern sighandler_t signal( int sig, sighandler_t );
-#define PATH_MAX MAX_PATH
+#define PATH_MAX 32768
 #endif
 
 #define STD