[Lldb-commits] [PATCH] D25681: [PseudoTerminal] Fix PseudoTerminal MSVC release crash
Ilod created this revision.
Ilod added reviewers: zturner, carlokok.
Ilod added a subscriber: lldb-commits.
Since r278177, the Posix functions in PseudoTerminal::OpenFirstAvailableMaster
on windows are now inlined LLVM_UNREACHABLE instead of return 0.
This causes __assume(0) to be inlined in OpenFirstAvailableMaster, allowing the
optimizer to change code because this function should never be executed. In
particular, on Visual 2015 Update 3 Win32 Release builds, the optimizer skips
the if (error_str) test, causing a crash if error_str is nullptr.
The added #if !defined(LLDB_DISABLE_POSIX) restore the previous behaviour
(which was doing nothing and returning true, as every function was returning 0,
and prevent crashes.
The crash was 100% when launching a test x86 executable (built with clang,
linked with lld-link) in lldb.
I don't know if there is another fix in progress (not calling the function on
Win32?), but it seems to be called from several places, so it may be simpler to
fix it in PseudoTerminal.
https://reviews.llvm.org/D25681
Files:
source/Utility/PseudoTerminal.cpp
Index: source/Utility/PseudoTerminal.cpp
===
--- source/Utility/PseudoTerminal.cpp
+++ source/Utility/PseudoTerminal.cpp
@@ -88,6 +88,7 @@
if (error_str)
error_str[0] = '\0';
+#if !defined(LLDB_DISABLE_POSIX)
// Open the master side of a pseudo terminal
m_master_fd = ::posix_openpt(oflag);
if (m_master_fd < 0) {
@@ -111,6 +112,7 @@
CloseMasterFileDescriptor();
return false;
}
+#endif
return true;
}
Index: source/Utility/PseudoTerminal.cpp
===
--- source/Utility/PseudoTerminal.cpp
+++ source/Utility/PseudoTerminal.cpp
@@ -88,6 +88,7 @@
if (error_str)
error_str[0] = '\0';
+#if !defined(LLDB_DISABLE_POSIX)
// Open the master side of a pseudo terminal
m_master_fd = ::posix_openpt(oflag);
if (m_master_fd < 0) {
@@ -111,6 +112,7 @@
CloseMasterFileDescriptor();
return false;
}
+#endif
return true;
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D25681: [PseudoTerminal] Fix PseudoTerminal MSVC release crash
Ilod updated this revision to Diff 75426.
Ilod added a comment.
Yeah, you're right, the previous code is used for reference was inconsistent
here. Now I return false and set an error string. Also remove an hacky ifdef
which was needed on previous implementation (because the master id was updated
to 0), but now we leave it to -1, so it's not needed anymore.
https://reviews.llvm.org/D25681
Files:
source/Utility/PseudoTerminal.cpp
Index: source/Utility/PseudoTerminal.cpp
===
--- source/Utility/PseudoTerminal.cpp
+++ source/Utility/PseudoTerminal.cpp
@@ -50,11 +50,7 @@
//--
void PseudoTerminal::CloseMasterFileDescriptor() {
if (m_master_fd >= 0) {
-// Don't call 'close' on m_master_fd for Windows as a dummy implementation of
-// posix_openpt above always gives it a 0 value.
-#ifndef _WIN32
::close(m_master_fd);
-#endif
m_master_fd = invalid_fd;
}
}
@@ -81,13 +77,14 @@
// file descriptor after this object is out of scope or destroyed.
//
// RETURNS:
-// Zero when successful, non-zero indicating an error occurred.
+// True when successful, false indicating an error occurred.
//--
bool PseudoTerminal::OpenFirstAvailableMaster(int oflag, char *error_str,
size_t error_len) {
if (error_str)
error_str[0] = '\0';
+#if !defined(LLDB_DISABLE_POSIX)
// Open the master side of a pseudo terminal
m_master_fd = ::posix_openpt(oflag);
if (m_master_fd < 0) {
@@ -113,6 +110,12 @@
}
return true;
+#else
+ if (error_str)
+::snprintf(error_str, error_len, "%s",
+ "pseudo terminal not supported");
+ return false;
+#endif
}
//--
@@ -124,7 +127,7 @@
// ReleaseSlaveFileDescriptor() member function.
//
// RETURNS:
-// Zero when successful, non-zero indicating an error occurred.
+// True when successful, false indicating an error occurred.
//--
bool PseudoTerminal::OpenSlave(int oflag, char *error_str, size_t error_len) {
if (error_str)
Index: source/Utility/PseudoTerminal.cpp
===
--- source/Utility/PseudoTerminal.cpp
+++ source/Utility/PseudoTerminal.cpp
@@ -50,11 +50,7 @@
//--
void PseudoTerminal::CloseMasterFileDescriptor() {
if (m_master_fd >= 0) {
-// Don't call 'close' on m_master_fd for Windows as a dummy implementation of
-// posix_openpt above always gives it a 0 value.
-#ifndef _WIN32
::close(m_master_fd);
-#endif
m_master_fd = invalid_fd;
}
}
@@ -81,13 +77,14 @@
// file descriptor after this object is out of scope or destroyed.
//
// RETURNS:
-// Zero when successful, non-zero indicating an error occurred.
+// True when successful, false indicating an error occurred.
//--
bool PseudoTerminal::OpenFirstAvailableMaster(int oflag, char *error_str,
size_t error_len) {
if (error_str)
error_str[0] = '\0';
+#if !defined(LLDB_DISABLE_POSIX)
// Open the master side of a pseudo terminal
m_master_fd = ::posix_openpt(oflag);
if (m_master_fd < 0) {
@@ -113,6 +110,12 @@
}
return true;
+#else
+ if (error_str)
+::snprintf(error_str, error_len, "%s",
+ "pseudo terminal not supported");
+ return false;
+#endif
}
//--
@@ -124,7 +127,7 @@
// ReleaseSlaveFileDescriptor() member function.
//
// RETURNS:
-// Zero when successful, non-zero indicating an error occurred.
+// True when successful, false indicating an error occurred.
//--
bool PseudoTerminal::OpenSlave(int oflag, char *error_str, size_t error_len) {
if (error_str)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D25681: [PseudoTerminal] Fix PseudoTerminal MSVC release crash
There is no new failure in the tests in Debug version on MSVC 2015, however a bunch of failures (~30) were masked on Release builds (the Test Methods count went from 769 to 1188). All the failures were already presents in the Debug builds (where the optimizer didn't mess up with the code due to LLVM_UNREACHERABLE). So I suppose it's still ok? (BTW, I don't have commit rights, so I will depend on someone to commit it) On Fri, Oct 21, 2016 at 8:11 PM, Zachary Turner wrote: > zturner accepted this revision. > zturner added a comment. > This revision is now accepted and ready to land. > > LGTM as long as you've run the test suite and confirmed everything works. > > > https://reviews.llvm.org/D25681 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D26209: Fix windows build after r285702, missing include to define PATH_MAX
Ilod created this revision.
Ilod added reviewers: labath, zturner, clayborg.
Ilod added a subscriber: lldb-commits.
Fix the windows build after r285702.
The removal of TimeValue.h inclusion removed the inner inclusion of PosixApi.h,
which defined PATH_MAX on windows, used by many other files.
Adding the PosixApi.h inclusion should not cause a problem (it only include
llvm/Support/Compiler.h on non-windows platform, and the needed file on windows
ones).
https://reviews.llvm.org/D26209
Files:
include/lldb/Host/FileSpec.h
Index: include/lldb/Host/FileSpec.h
===
--- include/lldb/Host/FileSpec.h
+++ include/lldb/Host/FileSpec.h
@@ -19,6 +19,7 @@
// Project includes
#include "lldb/Core/ConstString.h"
#include "lldb/Core/STLUtils.h"
+#include "lldb/Host/PosixApi.h"
#include "lldb/lldb-private.h"
namespace lldb_private {
Index: include/lldb/Host/FileSpec.h
===
--- include/lldb/Host/FileSpec.h
+++ include/lldb/Host/FileSpec.h
@@ -19,6 +19,7 @@
// Project includes
#include "lldb/Core/ConstString.h"
#include "lldb/Core/STLUtils.h"
+#include "lldb/Host/PosixApi.h"
#include "lldb/lldb-private.h"
namespace lldb_private {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
