hhellyer added a comment.
@labath - Yes sorry about that. To be safe I did the "honest" thing of using
arc to pull down the patch from phabriactor and then arc commit to land it to
make sure I committed exactly what was reviewed. Somewhere in that it lost the
contents of the core files and just
labath added a comment.
Oh, you fixed it. Thank you. :)
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath added a comment.
It fails on the linux buildbot, but not when I run it locally. I am going to
investigate the issue.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mai
hhellyer added a comment.
I'll land these changes tomorrow morning (UK time) so I can watch the builds
and check there's no problems with the tests.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://li
hhellyer added a comment.
I'll land these changes tomorrow morning (UK time) so I can watch the builds
and check there's no problems with the tests.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://li
hhellyer updated this revision to Diff 79056.
hhellyer added a comment.
Updating patch to remove executables and create empty targets in the test cases.
https://reviews.llvm.org/D26676
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/
packages/Python/lldbsuit
labath added a comment.
I just tried it, and these tests run fine without the executable file. Just
remove those, and I think we're ready.
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/TestGCore.py:38
+def do_test(self, filename, pid
hhellyer added a comment.
@labath If you are happy with the tests and the size of their supporting files
I'll land this, otherwise let me know if anything needs changing.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@l
hhellyer updated this revision to Diff 79051.
hhellyer added a comment.
Updated to add the test executables and core files.
https://reviews.llvm.org/D26676
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/gcore/
packages/Python/lldbsuite/test/functionalities/postmo
On 18 November 2016 at 19:54, Jim Ingham wrote:
> If we are going to do that, I wonder if we should start having tests share
> their inputs. We don’t do that for source files because the cost of
> duplication is so small, and then you don’t have the hassle of adding
> something to a source fie
If we are going to do that, I wonder if we should start having tests share
their inputs. We don’t do that for source files because the cost of
duplication is so small, and then you don’t have the hassle of adding something
to a source fie for test A messes up test B. But for binaries like core
labath added a comment.
We probably don't want to check in zillions of core files, but I believe having
a couple of them is fine, particularly when care is taken to minimize their
size. I believe the benefits (being able to run tests on a piece of code in a
reproducible and platform-independent
hhellyer updated this revision to Diff 78524.
hhellyer added a comment.
Updating the patch to include the test cases and scripts to produce the core
dumps.
I haven't included the core dumps and when I do I'll need to update the pid and
tid values in the test scripts. That can wait until we've dec
clayborg added a comment.
Checking core files into the repository will grow all git based repositories
over time as every version of every core file will be included in the git
clone. We have avoided checking in core files or any other larger files for
this very reason. Not sure what the right
hhellyer added a comment.
I'd assumed that gcore would ignore the coredump_filter setting but it turns
out it doesn't so I should be able to use that. lldb seems happy enough opening
the core files produced with a filter of 0 so it's probably the simplest
solution.
I don't think lldb has a cor
labath added a comment.
In https://reviews.llvm.org/D26676#598559, @hhellyer wrote:
> I haven't solved that yet! ;-)
>
> I'm currently ending up with cores of ~500kb which is probably too big. I'm
> seeing what I can do to bring them down but it might be that I can't shrink
> them that much. I
hhellyer added a comment.
I haven't solved that yet! ;-)
I'm currently ending up with cores of ~500kb which is probably too big. I'm
seeing what I can do to bring them down but it might be that I can't shrink
them that much. I'm attempting to write two tests, one for multiple threads and
one
labath added a comment.
I can't say we're being very consistent in enforcing it, but general llvm
policy is for tests to go together with the changes.
I am curious, how do you go about creating these core files?
The core files I created were very tiny as I did not require any syscalls, so I
co
hhellyer added a comment.
This is marked ready to land and I can land it now but I'm still working on the
testcases.
I can either:
- Land the code changes and do the tests under another patch.
- Hold off on delivering this until I've finished the tests.
I'm not sure which is the preferred optio
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/l
hhellyer updated this revision to Diff 78173.
hhellyer added a comment.
Update patch due to two missing changes.
https://reviews.llvm.org/D26676
Files:
source/Plugins/Process/elf-core/ProcessElfCore.cpp
source/Plugins/Process/elf-core/ThreadElfCore.cpp
source/Plugins/Process/elf-core/Thre
hhellyer updated this revision to Diff 78163.
hhellyer added a comment.
Update with fixes from review comments.
I've run clang-format, fixed the use of iterators and added const qualifiers.
I removed the use of the SIGSTOP constant and use GetUnixSignals instead.
I've also moved that block down a
labath added inline comments.
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:224
+ // If all else fails force the first thread to be SIGSTOP
+ m_thread_data.begin()->signo = SIGSTOP;
+}
You should not use signal numbers from the host
sas added a comment.
Just a couple nits inline. Also, did you run `clang-format` on your change? I
see some issues with `if`s and the associated parentheses.
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:215
+ }
+ if (siginfo_signal_found == false) {
+//
jingham added a comment.
Besides Greg's comments, this looks reasonable to me.
https://reviews.llvm.org/D26676
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath added a comment.
Thank you for looking into this. This has been a long standing issue that we
haven't got time to address.
Could you also add some tests to cover the new functionality? It sounds like it
would be easy to generate tiny core files which trigger this. You can look at
tests
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Just a few quick changes.
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:208-209
+ // Check we found a signal in a SIGINFO note.
+ for (std::v
hhellyer created this revision.
hhellyer added a subscriber: lldb-commits.
This patch changes the way ProcessElfCore.cpp handles signal information.
The patch changes ProcessElfCore.cpp to use the signal from si_signo in SIGINFO
notes in preference to the value of cursig in PRSTATUS notes. The va
28 matches
Mail list logo
