[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
jankratochvil added a comment. In https://reviews.llvm.org/D32167#1024546, @labath wrote: > I personally don't think having a new debug info flavour is a good idea. > Tests written specifically to test this functionality will be easier to > maintain and debug when they break. And keeping adding new flavours for every > compiler option is not a viable long term strategy. Here I disagree, from GDB testsuite experience I find more wider testing always better as it will better catch bugs even in unrelated new functionalities being implemented in the future. One can never guess all the combinations in advance - if one could then one needs no regression testing at all. There are more than enough CPU cycles available for testing. I find rather a serious problem racy==unreliable test results which waste human efforts investigating the results and with more parallel testing (make -j) and virtualized infrastructure the racy results happen even more often. As an example one racy testcase I idenitifed is: packages/Python/lldbsuite/test/functionalities/signal/handle-segv/TestHandleSegv.py line 33: AssertionError: 4 != 5 (eStateLaunching != eStateStopped) I think at least these test are racy: BreakpointAfterJoinTestCase-test CreateDuringStepTestCase-test_step_inst_with HandleSegvTestCase-test_inferior_handle_sigsegv ReturnValueTestCase-test_vector_values TestTargetSymbolsBuildidSymlink-test_target_symbols_buildid ThreadSpecificBreakPlusConditionTestCase-test_python ThreadSpecificBreakTestCase-test_python ThreadStateTestCase-test_process_interrupt https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
davide added a comment. In https://reviews.llvm.org/D32167#1026762, @jankratochvil wrote: > In https://reviews.llvm.org/D32167#1024546, @labath wrote: > > > I personally don't think having a new debug info flavour is a good idea. > > Tests written specifically to test this functionality will be easier to > > maintain and debug when they break. And keeping adding new flavours for > > every compiler option is not a viable long term strategy. > > > Here I disagree, from GDB testsuite experience I find more wider testing > always better as it will better catch bugs even in unrelated new > functionalities being implemented in the future. One can never guess all the > combinations in advance - if one could then one needs no regression testing > at all. There are more than enough CPU cycles available for testing. I find > rather a serious problem racy==unreliable test results which waste human > efforts investigating the results and with more parallel testing (make -j) > and virtualized infrastructure the racy results happen even more often. As > an example one racy testcase I idenitifed is: > > > packages/Python/lldbsuite/test/functionalities/signal/handle-segv/TestHandleSegv.py > line 33: AssertionError: 4 != 5 (eStateLaunching != eStateStopped) > > > I think at least these test are racy: > > BreakpointAfterJoinTestCase-test >CreateDuringStepTestCase-test_step_inst_with >HandleSegvTestCase-test_inferior_handle_sigsegv >ReturnValueTestCase-test_vector_values >TestTargetSymbolsBuildidSymlink-test_target_symbols_buildid >ThreadSpecificBreakPlusConditionTestCase-test_python >ThreadSpecificBreakTestCase-test_python >ThreadStateTestCase-test_process_interrupt > I'm not necessarily convinced having another column in the matrix is something that can substitute targeted testing. If you're talking about something complementary, I think we might consider it, as a follow up. Also, thanks for bringing the problem of the racy tests up. Adding another row in the matrix increases the chances of test failures. I think we might consider addressing the unreliability before making such a big change. Do you have a way of reproducing? [or even better, investigating] Our bots have been green for a while (we tried to investigate known issues http://lab.llvm.org:8080/green/view/LLDB/) but we can only address what we know :) https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
jankratochvil added a comment.
In https://reviews.llvm.org/D32167#1026779, @davide wrote:
> Do you have a way of reproducing?
It just happens for me each time - on Fedora 27 x86_64 on 16-core (32HT) 2-node
NUMA machine having env var `MAKEFLAGS=-j32` (and `tuned-adm
throughput-performance` if it matters). Comparing multiple runs makes some
tests {Expected,Unexpected,}{Success,Failure,Error} without changing anything.
> [or even better, investigating]
When the testcase is then ran separately it sure succeeds as the machine then
has no load that time. Maybe one could make some race reproducer helper (such
as was GDB read1 ) but I
should analyse more LLDB testsuite races to know the common racy issues in LLDB
testsuite.
https://reviews.llvm.org/D32167
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
davide added a comment.
In https://reviews.llvm.org/D32167#1026780, @jankratochvil wrote:
> In https://reviews.llvm.org/D32167#1026779, @davide wrote:
>
> > Do you have a way of reproducing?
>
>
> It just happens for me each time - on Fedora 27 x86_64 on 16-core (32HT)
> 2-node NUMA machine having env var `MAKEFLAGS=-j32` (and `tuned-adm
> throughput-performance` if it matters). Comparing multiple runs makes some
> tests {Expected,Unexpected,}{Success,Failure,Error} without changing anything.
>
> > [or even better, investigating]
>
> When the testcase is then ran separately it sure succeeds as the machine then
> has no load that time. Maybe one could make some race reproducer helper (such
> as was GDB read1 ) but
> I should analyse more LLDB testsuite races to know the common racy issues in
> LLDB testsuite.
That sounds a great helper. Any chance you're interested in working on that? I
don't have access to a machine beefy enough to repro the failures, I'm afraid.
https://reviews.llvm.org/D32167
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
jankratochvil added a comment. In https://reviews.llvm.org/D32167#1026781, @davide wrote: > Any chance you're interested in working on that? Yes, it is in the shortterm or rather midterm plan. https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44081: [lldb] Add synthetic frontend for _NSCallStackArray
kubamracek created this revision.
kubamracek added reviewers: jingham, jasonmolenda, davide.
An Obj-C array type _NSCallStackArray is used in NSException backtraces. This
patch adds a synthetic frontend for _NSCallStackArray, which now correctly
returns frame PCs.
https://reviews.llvm.org/D44081
Files:
packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
source/Plugins/Language/ObjC/NSArray.cpp
source/Plugins/Language/ObjC/ObjCLanguage.cpp
Index: source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -412,6 +412,9 @@
AddCXXSummary(
objc_category_sp, lldb_private::formatters::NSArraySummaryProvider,
"NSArray summary provider", ConstString("__NSCFArray"), appkit_flags);
+ AddCXXSummary(
+ objc_category_sp, lldb_private::formatters::NSArraySummaryProvider,
+ "NSArray summary provider", ConstString("_NSCallStackArray"), appkit_flags);
AddCXXSummary(
objc_category_sp, lldb_private::formatters::NSArraySummaryProvider,
"NSArray summary provider", ConstString("CFArrayRef"), appkit_flags);
@@ -531,6 +534,10 @@
lldb_private::formatters::NSArraySyntheticFrontEndCreator,
"NSArray synthetic children", ConstString("__NSCFArray"),
ScriptedSyntheticChildren::Flags());
+ AddCXXSynthetic(objc_category_sp,
+ lldb_private::formatters::NSArraySyntheticFrontEndCreator,
+ "NSArray synthetic children", ConstString("_NSCallStackArray"),
+ ScriptedSyntheticChildren::Flags());
AddCXXSynthetic(objc_category_sp,
lldb_private::formatters::NSArraySyntheticFrontEndCreator,
"NSArray synthetic children",
Index: source/Plugins/Language/ObjC/NSArray.cpp
===
--- source/Plugins/Language/ObjC/NSArray.cpp
+++ source/Plugins/Language/ObjC/NSArray.cpp
@@ -218,6 +218,25 @@
}
+namespace CallStackArray {
+struct DataDescriptor_32 {
+ uint32_t _data;
+ uint32_t _used;
+ uint32_t _offset;
+ const uint32_t _size = 0;
+};
+
+struct DataDescriptor_64 {
+ uint64_t _data;
+ uint64_t _used;
+ uint64_t _offset;
+ const uint64_t _size = 0;
+};
+
+using NSCallStackArraySyntheticFrontEnd =
+GenericNSArrayMSyntheticFrontEnd;
+} // namespace CallStackArray
+
template
class GenericNSArrayISyntheticFrontEnd : public SyntheticChildrenFrontEnd {
public:
@@ -368,6 +387,7 @@
static const ConstString g_NSArrayCF("__NSCFArray");
static const ConstString g_NSArrayMLegacy("__NSArrayM_Legacy");
static const ConstString g_NSArrayMImmutable("__NSArrayM_Immutable");
+ static const ConstString g_NSCallStackArray("_NSCallStackArray");
if (class_name.IsEmpty())
return false;
@@ -423,6 +443,12 @@
valobj_addr + 2 * ptr_size, ptr_size, 0, error);
if (error.Fail())
return false;
+ } else if (class_name == g_NSCallStackArray) {
+Status error;
+value = process_sp->ReadUnsignedIntegerFromMemory(
+valobj_addr + 2 * ptr_size, ptr_size, 0, error);
+if (error.Fail())
+ return false;
} else {
auto &map(NSArray_Additionals::GetAdditionalSummaries());
auto iter = map.find(class_name), end = map.end();
@@ -817,6 +843,7 @@
static const ConstString g_NSArray1("__NSSingleObjectArrayI");
static const ConstString g_NSArrayMLegacy("__NSArrayM_Legacy");
static const ConstString g_NSArrayMImmutable("__NSArrayM_Immutable");
+ static const ConstString g_NSCallStackArray("_NSCallStackArray");
if (class_name.IsEmpty())
return nullptr;
@@ -846,6 +873,8 @@
return (new Foundation1010::NSArrayMSyntheticFrontEnd(valobj_sp));
else
return (new Foundation109::NSArrayMSyntheticFrontEnd(valobj_sp));
+ } else if (class_name == g_NSCallStackArray) {
+return (new CallStackArray::NSCallStackArraySyntheticFrontEnd(valobj_sp));
} else {
auto &map(NSArray_Additionals::GetAdditionalSynthetics());
auto iter = map.find(class_name), end = map.end();
Index: packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
===
--- packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
+++ packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
@@ -28,7 +28,8 @@
self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
substrs=['stopped', 'stop reason = breakpoint'])
-thread = self.dbg.GetSelectedTarget().GetProcess().GetSelectedThread()
+target = self.dbg.GetSelectedTarget()
+thread = target.GetProcess().GetSelectedThread()
frame = thread.GetSelectedFrame()
self.expect(
@@ -87,4 +88,14 @@
self.assertEqual(userInfo.summary, "1 key/value pair")
[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values
kubamracek added a comment. The changes for _NSCallStackArray are at https://reviews.llvm.org/D44081. https://reviews.llvm.org/D43884 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44056: [lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel
labath added inline comments.
Comment at: source/Host/posix/HostThreadPosix.cpp:44
if (IsJoinable()) {
#ifndef __ANDROID__
#ifndef __FreeBSD__
xiaobai wrote:
> labath wrote:
> > xiaobai wrote:
> > > aprantl wrote:
> > > > What about:
> > > > ```
> > > > #ifdef __ANDROID__
> > > > error.SetErrorString("HostThreadPosix::Cancel() not supported on
> > > > Android");
> > > > #else
> > > > #ifdef __FreeBSD__
> > > > int err = ::pthread_cancel(m_thread);
> > > > error.SetError(err, eErrorTypePOSIX);
> > > > #else
> > > > llvm_unreachable("someone is calling HostThread::Cancel()");
> > > > #endif
> > > > #endif
> > > > }
> > > > return error;
> > > > }
> > > > ```
> > > I agree with Adrian's suggestion, but I would add that you can remove one
> > > of the `#endif` if you use `#elif defined(__FreeBSD__)` instead of an
> > > `#else` + `#ifdef __FreeBSD__`.
> > I think we can just unify the __ANDROID__ and __FreeBSD__ cases (turn both
> > into unreachable). We only run lldb-server on android, and there we're
> > mostly single-threaded, so there shouldn't be any thread cancelling going
> > on anyway...
> I don't think we can make the FreeBSD case unreachable (if I understand the
> code correctly) since that's the one case when `::pthread_cancel` is actually
> getting called.
>
> If we can make the `__ANDROID__` case unreachable, this would very easily
> turn into
> ```
> #if defined(__FreeBSD__) || defined(__NetBSD__)
> int err = ::pthread_cancel(m_thread);
> error.SetError(err, eErrorTypePOSIX);
> #else
> llvm_unreachable("Someone is calling HostThreadPosix::Cancel()");
> #endif
> ```
>
> I'm somewhat unfamiliar with how exactly this code is used on android. If I
> understand correctly, you're saying it's used in lldb-server and you meant
> that lldb-server is mostly single threaded so this code shouldn't get run
> there?
Right, sorry, I misinterpreted the ifdefs. We should then merge the android
case into the !FreeBsd case like you suggest (although I'm not sure why NetBSD
has appeared there now).
https://reviews.llvm.org/D44056
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44056: [lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel
xiaobai added inline comments.
Comment at: source/Host/posix/HostThreadPosix.cpp:44
if (IsJoinable()) {
#ifndef __ANDROID__
#ifndef __FreeBSD__
labath wrote:
> xiaobai wrote:
> > labath wrote:
> > > xiaobai wrote:
> > > > aprantl wrote:
> > > > > What about:
> > > > > ```
> > > > > #ifdef __ANDROID__
> > > > > error.SetErrorString("HostThreadPosix::Cancel() not supported on
> > > > > Android");
> > > > > #else
> > > > > #ifdef __FreeBSD__
> > > > > int err = ::pthread_cancel(m_thread);
> > > > > error.SetError(err, eErrorTypePOSIX);
> > > > > #else
> > > > > llvm_unreachable("someone is calling HostThread::Cancel()");
> > > > > #endif
> > > > > #endif
> > > > > }
> > > > > return error;
> > > > > }
> > > > > ```
> > > > I agree with Adrian's suggestion, but I would add that you can remove
> > > > one of the `#endif` if you use `#elif defined(__FreeBSD__)` instead of
> > > > an `#else` + `#ifdef __FreeBSD__`.
> > > I think we can just unify the __ANDROID__ and __FreeBSD__ cases (turn
> > > both into unreachable). We only run lldb-server on android, and there
> > > we're mostly single-threaded, so there shouldn't be any thread cancelling
> > > going on anyway...
> > I don't think we can make the FreeBSD case unreachable (if I understand the
> > code correctly) since that's the one case when `::pthread_cancel` is
> > actually getting called.
> >
> > If we can make the `__ANDROID__` case unreachable, this would very easily
> > turn into
> > ```
> > #if defined(__FreeBSD__) || defined(__NetBSD__)
> > int err = ::pthread_cancel(m_thread);
> > error.SetError(err, eErrorTypePOSIX);
> > #else
> > llvm_unreachable("Someone is calling HostThreadPosix::Cancel()");
> > #endif
> > ```
> >
> > I'm somewhat unfamiliar with how exactly this code is used on android. If I
> > understand correctly, you're saying it's used in lldb-server and you meant
> > that lldb-server is mostly single threaded so this code shouldn't get run
> > there?
> Right, sorry, I misinterpreted the ifdefs. We should then merge the android
> case into the !FreeBsd case like you suggest (although I'm not sure why
> NetBSD has appeared there now).
Ah, I added NetBSD since @krytarowski pointed out NetBSD has `pthread_cancel`
available as well. However that probably prevents this patch from being an NFC,
so that could probably be added separately.
https://reviews.llvm.org/D44056
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D44056: [lldb] Fix "code unreachable" warning in HostThreadPosix::Cancel
labath added inline comments.
Comment at: source/Host/posix/HostThreadPosix.cpp:44
if (IsJoinable()) {
#ifndef __ANDROID__
#ifndef __FreeBSD__
xiaobai wrote:
> labath wrote:
> > xiaobai wrote:
> > > labath wrote:
> > > > xiaobai wrote:
> > > > > aprantl wrote:
> > > > > > What about:
> > > > > > ```
> > > > > > #ifdef __ANDROID__
> > > > > > error.SetErrorString("HostThreadPosix::Cancel() not supported
> > > > > > on Android");
> > > > > > #else
> > > > > > #ifdef __FreeBSD__
> > > > > > int err = ::pthread_cancel(m_thread);
> > > > > > error.SetError(err, eErrorTypePOSIX);
> > > > > > #else
> > > > > > llvm_unreachable("someone is calling HostThread::Cancel()");
> > > > > > #endif
> > > > > > #endif
> > > > > > }
> > > > > > return error;
> > > > > > }
> > > > > > ```
> > > > > I agree with Adrian's suggestion, but I would add that you can remove
> > > > > one of the `#endif` if you use `#elif defined(__FreeBSD__)` instead
> > > > > of an `#else` + `#ifdef __FreeBSD__`.
> > > > I think we can just unify the __ANDROID__ and __FreeBSD__ cases (turn
> > > > both into unreachable). We only run lldb-server on android, and there
> > > > we're mostly single-threaded, so there shouldn't be any thread
> > > > cancelling going on anyway...
> > > I don't think we can make the FreeBSD case unreachable (if I understand
> > > the code correctly) since that's the one case when `::pthread_cancel` is
> > > actually getting called.
> > >
> > > If we can make the `__ANDROID__` case unreachable, this would very easily
> > > turn into
> > > ```
> > > #if defined(__FreeBSD__) || defined(__NetBSD__)
> > > int err = ::pthread_cancel(m_thread);
> > > error.SetError(err, eErrorTypePOSIX);
> > > #else
> > > llvm_unreachable("Someone is calling HostThreadPosix::Cancel()");
> > > #endif
> > > ```
> > >
> > > I'm somewhat unfamiliar with how exactly this code is used on android. If
> > > I understand correctly, you're saying it's used in lldb-server and you
> > > meant that lldb-server is mostly single threaded so this code shouldn't
> > > get run there?
> > Right, sorry, I misinterpreted the ifdefs. We should then merge the android
> > case into the !FreeBsd case like you suggest (although I'm not sure why
> > NetBSD has appeared there now).
> Ah, I added NetBSD since @krytarowski pointed out NetBSD has `pthread_cancel`
> available as well. However that probably prevents this patch from being an
> NFC, so that could probably be added separately.
I see. Others systems have pthread_cancel as well (android is the main
exception here), but that function is very c++-unfriendly, so I think we should
stop using it (and that's probably the reason why we have the llvm_unreachable
there in the first place).
https://reviews.llvm.org/D44056
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
