- User Since
- Jun 4 2013, 6:02 AM (202 w, 6 d)
Address Tamas's comments.
We don't support running the test suite on Windows with MSVC. We run it
with clang targeting windows instead. So anyone running the test suite on
Windows is already using clang, and we can just specify a linux triple to
get an ELF binary.
Thanks for the patch. Could you please also add an appropriate test for it? Doing something similar to what packages/Python/lldbsuite/test/functionalities/frame_var/TestFrameVar.py does should be the easiest way to test this.
Fri, Apr 21
Thank you for looking at this. My response is below.
Looks good, thank you.
Thu, Apr 20
Jim, Jason, could one of you take a look at this please?
A test would infinitely times more valuable then a demo script. What is the tiniest core file you can produce on NetBSD? (on linux we've gotten them down to about 20K) Then we could check that in and write a test for it...
Thanks for digging into this, I've learned something new today. Fixing this with a cast seems reasonable then.
Wed, Apr 19
lgtm. You might consider using llvm::formatv instead of the raw snprintf -- it would make the PRIx64 macros more readable.
Also remove the skipIfDarwin decorators, as they should work now -- there was
nothing darwin-specific about them, just our bots don't run assert builds at the
This is the list I am getting when using ToT clang targetting x86_64 linux.
It looks like you had a fun day yesterday. :) Unfortunately, I have to add to your problems, as I've had to revert your commit due to a fairly big problem.
Tue, Apr 18
The new tests which are not passing need to be disabled -- we cannot have a red tree while this is being developed.
I am confused. m_registers_count is declared as uint8_t at RegisterContextPOSIX_mips64.h:67...
Sat, Apr 15
Thank you for taking the time to do this.
looks good, thank you.
Thu, Apr 13
Wed, Apr 12
One extra thing you should be aware of is that unlike poll(2), WSAPoll only works on sockets. We currently have pieces of code ifdef-ed out on windows because we have no way to wait on pipes, and eventually I'd like to teach the MainLoop to do that as well. I'm not saying we should do that now, just pointing out that the implementations are likely to diverge in the future.
Thank you very much. I am soo happy that these files are going away.
Tue, Apr 11
Address review comments.
This is fine, as far as i am concerned.
When I created the MainLoop class, i was hoping it would become the central place for all select-like functionality. Currently it uses pselect, but i don't see a reason we couldn't switch it to ppoll (but we actually don't have to, as the current implementation should work just fine for your use case). We also can-but-don't-have-to provide a specialized kevent-based implementation for mac&freebsd (afaict, kevent also supports listening for signals, but i would be fine with leaving that out even, as we don't need that functionality on these platforms). So all that needs to be added is a WSAPoll-based MainLoopWindows.
This makes all sorts of tests fail. Regardless of your platform you should at least be able to reproduce the following failures:
FAIL: test_FPR_SSE (functionalities/postmortem/elf-core/TestLinuxCore.py) FAIL: test_image_list_shows_multiple_architectures (functionalities/object-file/TestImageListMultiArchitecture.py) FAIL: test_local_variables_in_minidump (functionalities/postmortem/minidump-new/TestMiniDumpNew.py) ERROR: test_deeper_stack_in_minidump (functionalities/postmortem/minidump-new/TestMiniDumpNew.py) ERROR: test_deeper_stack_in_minidump_with_same_pid_running (functionalities/postmortem/minidump-new/TestMiniDumpNew.py) ERROR: test_two_cores_same_pid (functionalities/postmortem/minidump-new/TestMiniDumpNew.py)
Mon, Apr 10
Remove the vector<bool> summary provider as well (picked up from Tamas's version of the patch).
Wed, Apr 5
Fri, Mar 31
Thank you for adding the test. Lgtm, assuming the second getmemoryregioninfo call is accidental.
Thu, Mar 30
Thank you. Keep up the good work.
I cant help but feel that this could have been done in a simpler way, but then again, some of the cases you have dug up are quite tricky.
I think we should do some performance measurements to see whether this needs more optimising or it's fine as is.
The additional check sounds fine. There's a test for this class in unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp where you can add a test for this behavior.
Wed, Mar 29
I'm not sure why you ended up here. I think you have too wide phabricator filter somewhere. :)
We don't depend on the RuntimeDyld component of llvm directy -- we only use it indirectly through the ExecutionEngine component. Shouldn't that be reflected as a dependency in the build system somehow, so that the former can be pulled in directly ?
RuntimeDyld is listed as a dependency of ExecutionEngine in lib/ExecutionEngine/LLVMBuild.txt, but that does not seem to be reflected in the cmake? Is that a bug on the llvm side?
Tue, Mar 28
I wasn't able to go into this too deeply, but here is what I have after a quick pass. I won't be able to review this thoroughly that soon, but I think it can go in after you take my comments into consideration.
I am afraid I will be away for two weeks as well.. :(
Sun, Mar 26
This doesn't bother me too much, but i'm curious about how are you getting anything reasonable out of the test suite without a working compiler (?)
Mar 23 2017
Does anyone object to me landing this? I am going to be careful and wait for the change to trickle through CI before submitting any followup changes.
Ah, that explains it, thanks.
Thank you for that.
BTW, thank you for adding the test. It's not obvious from the patch: how big are the core files?
Mar 22 2017
Mar 21 2017
I think you should group these "add netbsd to a list" type of changes into single diff. There's not point in reviewing each separately.
Heh... I did not expect it would get this small, but ok. :)