2014-11-13 0:40 GMT+03:00 Joseph Myers <jos...@codesourcery.com>: > On Wed, 12 Nov 2014, Ilya Enkovich wrote: > >> MPX runtime needs to be linked with programs using MPX because it >> initializes hardware. Without it all MPX instructions are just NOPs. >> Thus it's not an extra functionality, but is for basic MPX >> functionality. > > So what if you just have the constructor that initializes the hardware - > could everything else (handling environment variables, printing more > detailed diagnostics, etc.) be separate? How much is basic MPX > functionality, how much is extra? Basic functionality should arguably be > like libgcc in namespace terms (so not calling any libc functions outside > of ISO C90 namespace, using reserved-namespace versions such as __write > instead if necessary and if such versions are available); extra > functionality need not be so restricted.
It's hard to decide which of runtime functionality should be considered as basic and how it should be used. We may say that the only basic thing is hardware enabling which is enable_mpx and stop here. But then you get minimal but quite useless library. Yes, it can enable MPX and thus make bounds violation to interrupt a program. But users cannot enable/disable MPX dinamycally then. Also they cannot configure it. Thus either control via environmental variables appears in this core library or we transform initialization function from constructor to interface function and use it from another extended MPX library which support environment variables, logging etc. But the core library will only be used by this extended MPX library and nothing else. So why not to leave it as a single library as it is? > >> I also fixed other issues you mentioned in your previous comments. >> Below is a new version. Does it look better? > > I don't see anything obvious to do symbol versioning. > > If this isn't providing interfaces for the user program or > compiler-generated code to call, then the symbol versioning could hide all > symbols (so there are no exported symbols at all and the library operates > purely through its initialization constructor). Or build with > -fvisibility=hidden to hide the symbols that way. Either way, the dynamic > symbol table of the shared library would end up with just undefined > symbols, nothing exported by the library. Currently it's just constructors. I'll hide functions. > > The issue with signal handlers calling inappropriate functions still > applies - you're calling vfprintf from a signal handler. Using > pthread_mutex_lock around it doesn't help at all; vfprintf can call > malloc, and the signal may have interrupted malloc, for example. You > really can't use stdio at all inside signal handlers - if you want to do > I/O in them, you have to use write (). (There's an argument that dprintf > or an snprintf/write combination ought to be AS-safe, but currently they > aren't; see <https://sourceware.org/bugzilla/show_bug.cgi?id=16060>; > snprintf is probably safe than dprintf in practice.) I'll get rid of printf there. Will use write + own int to string convert to avoid snprintf. > > I mentioned the question of static writable variables. If those variables > are only ever modified at startup, before any threads have been created, > could you add comments to that effect (and otherwise ensure comments on > the variables explain how you ensure they are accessed in a thread-safe > way)? Almost all varibles are written at process initialization only. [out|err]_file_dirty may be written by different threads in __mpxrt_print but the same value is always written and value is read in process finalizer only. Since __mpxrt_print has mutex lock anyway, I may move the rest of fucntion body under the lock. Remaining var is bounds violation counter num_bnd_chk which is written in signal handler only and read in the process finalizer. So I believe all vars are thread safe. Will add appropriate comments in the code. Thanks a lot for detailed review! Ilya > > -- > Joseph S. Myers > jos...@codesourcery.com
