This is an archive of the discontinued LLVM Phabricator instance.

Centralize all calls to select() into a single class so we always call select properly
ClosedPublic

Authored by clayborg on Jul 28 2016, 5:21 PM.

Details

Summary

New calls to select() were introduced into LLDB over the past year that were not doing the right thing on Darwin and these were causing crashes when the file descriptor exceeded FD_SETSIZE. Darwin has special abilities to deal with file descriptors that are greater than or equal to FD_SETSIZE, so we should be able to do things correctly.

This change makes a new File::SelectInfo class that helps encapsulate calls to select() so they all happen correctly. I have updates all calls to select() within LLDB to use this code.

This needs to be tested on Windows, Linux, and FreeBSD. If anyone can't test in the next few days, please resign as reviewer and add someone that can.

Diff Detail

Event Timeline

clayborg updated this revision to Diff 66063.Jul 28 2016, 5:21 PM
clayborg retitled this revision from to Centralize all calls to select() into a single class so we always call select properly.
clayborg updated this object.
clayborg added reviewers: zturner, labath, emaste.
clayborg added a subscriber: Restricted Project.
emaste edited edge metadata.Jul 29 2016, 6:08 AM

Testing now on FreeBSD. Do you have a sense of how many fds lldb might use in practice? For FreeBSD we can increase it from the default if necessary via

NOTES
     The default size of FD_SETSIZE is currently 1024.  In order to accommo‐
     date programs which might potentially use a larger number of open files
     with select(), it is possible to increase this size by having the program
     define FD_SETSIZE before the inclusion of any header which includes
     <sys/types.h>.

Also for future patch uploads would you include more context to make review easier (e.g. git diff -U999 or svn diff --diff-cmd=diff -x -U999)?

I saw no regression on FreeBSD with this patch

labath resigned from this revision.Jul 29 2016, 8:31 AM
labath edited reviewers, added: tberghammer; removed: labath.

I see no regressions on Linux. Reassigning to Tamas, if you need any more input, as I will be away next week.

I don't want to block this on that, as I think it is a step in the right direction, but am thinking about the interaction of this and the MainLoopPosix class I added last year, which I envisioned as doing the same thing. I think they should be eventually merged, or at least implemented in terms of each other. The current differences are :

  • SelectInfo does not handle signals -- pselect (hard requirement for lldb-server). They don't need to work on windows though. I assume the darwin select extension trick will work on pselect as well(?)
  • MainLoop uses a callback-based interface - might be more cumbersome for some uses, but some of the uses I see here would actually benefit from that, as they are doing a while(!done) loop anyway.
include/lldb/Host/File.h
442 ↗(On Diff #66063)

I am not sure about the location of this class, File seems like a bad place, especially considering that select does not even work with files (sockets only) on Windows. Maybe IOObject.h, or a standalone file?

501 ↗(On Diff #66063)

I think we use _up as a suffix now.

zturner added a subscriber: lldb-commits.
labath added a subscriber: labath.Jul 29 2016, 9:58 AM

Ok, there is one more big difference: MainLoop doesn't have timeouts yet. So yes, I guess these are not yet ready for merging...

zturner edited edge metadata.Jul 29 2016, 10:11 AM

Currently running the test suite, will report back when it's done.

include/lldb/Host/File.h
442 ↗(On Diff #66063)

+1 for making a new file out of this. I would probably just call it Select.h or SelectInfo.h. I like Select better than SelectInfo (as a file name and as a class name), because SelectInfo makes it sound like you're going to query the class for something, not that it's actually going to perform some system level operation.

493–498 ↗(On Diff #66063)

How about a bitfield?

500 ↗(On Diff #66063)

llvm::DenseMap will be more efficient (space-wise, and speed-wise)

501 ↗(On Diff #66063)

llvm::Optional<T> is another option here. It's nice in that it doesn't use a heap allocation and has better cache performance.

source/Host/common/File.cpp
19 ↗(On Diff #66063)

Inside of the #if defined(_WIN32) preprocessor section here, you need to add this line:

#include <winsock2.h>

Aside from the compilation error caused by not including winsock2.h, I saw no other obvious problems here, and there are no test regressions.

Testing now on FreeBSD. Do you have a sense of how many fds lldb might use in practice? For FreeBSD we can increase it from the default if necessary via

NOTES
     The default size of FD_SETSIZE is currently 1024.  In order to accommo‐
     date programs which might potentially use a larger number of open files
     with select(), it is possible to increase this size by having the program
     define FD_SETSIZE before the inclusion of any header which includes
     <sys/types.h>.

Also for future patch uploads would you include more context to make review easier (e.g. git diff -U999 or svn diff --diff-cmd=diff -x -U999)?

This is a particular problem on OS X because we support debugging with the debug info in .o files, so for a complex project we could be opening lots of those files. If you don't do that, then it's more like number of shared libraries a normal program will open. That's unlikely to get to more than 1024.

emaste added a comment.Aug 2 2016, 1:57 PM

This is a particular problem on OS X because we support debugging with the debug info in .o files, so for a complex project we could be opening lots of those files. If you don't do that, then it's more like number of shared libraries a normal program will open. That's unlikely to get to more than 1024.

OK. We'll want to support the same approach in FreeBSD too, but other tooling needs to be in place first (at least a new linker) so it can be left alone for now. We can just bump it up if/when it becomes a problem.

I am back from vacation again.

I will place this in a stand alone file and I will use SelectInfo.h if there is a "select.h" already in any "include" directories like "/usr/include" so we don't run into problems including the right file. If there aren't any select.h files, I will use Select.h. I would like to not use anything llvm in the SelectInfo.h file to keep us able to use this class in other tools that might not be including llvm or clang.

clayborg updated this revision to Diff 67253.Aug 8 2016, 5:05 PM
clayborg edited edge metadata.

Updated with all comments taken into account. Zach, please try this out and let me know if it compiles.

I used SelectHelper.h because there is a "/usr/include/select.h" header file and case insensitive file systems would have issues if I used Select.h.

clayborg accepted this revision.Aug 10 2016, 3:52 PM
clayborg added a reviewer: clayborg.

This was committed with 278299. We can iterate on this as needed.

This revision is now accepted and ready to land.Aug 10 2016, 3:52 PM
clayborg closed this revision.Aug 10 2016, 3:52 PM