2013/7/25 Joseph S. Myers <jos...@codesourcery.com>: > On Wed, 24 Jul 2013, Ilya Enkovich wrote: > >> Well, this patch does not introduce any changes on user-visible level. >> It just adds MPX instructions support to i386 target. Usually each new >> x86 instruction has corresponding builtin function and therefore is >> provided with a testcase. But MPX instructions do not have builtin >> function for direct generation and therefore there are no tests for >> this patch. > > Usually also new instructions have a -m option to enable them, but you > don't have that here either. I realise the instructions are NOPs on > processors not supporting them (all processors not supporting them?), but > given that the availability of the instructions must at some point affect > either the semantics of RTL containing checks, or what RTL the compiler > can generate code for, I'd think you should have such an option anyway.
We had this option initially but removed it because it was useless. You get nothing when pass -mmpx and you always have to to pass it when pass -fmpx. Introduction of that option just makes you to always use pair '-fmpx -mmpx' which is not convenient. > >> I have subsequent patches to add user-visible features based on MPX. >> Sorry for missing series indicator (fixed). High-level feature based >> on MPX is described on GCC Wiki >> (http://gcc.gnu.org/wiki/Intel%20MPX%20support%20in%20the%20GCC%20compiler). > > Thanks. You also need to post the implementation design in terms of > semantics of machine-independent tree/GIMPLE/RTL additions (which of > course should be added to the relevant .texi files). Could you please be more specific regarding .texi files? In my implementation I add - new type - new modes - new options - new builtins - new target hooks I know where hooks and options should be documented. But what about others? For now I saw only description in comments. tree-mpx.c files has a description in a header. Does it look close to implementation design you are talking about? > > Having options starting -fmpx seems odd. If something is > machine-independent, as using -f options implies, then the options should > have a machine-independent name rather than being named after one > particular implementation. Given we already have a -fbounds-check option > (documented as supported for Java and Fortran only), that seems an obvious > possibility for naming. If for some reason it's a bad idea to use that > option, at least the option names should make clear the similarities and > differences from that option. More generally, the patch series submission > should include a comparison with other bounds-checking features in GCC > such as -fmudflap and -fsanitize=address (and the user documentation > should enable users to understand the advantages and disadvantages of > each, so they can choose which is the best match to their requirements). > I'm far from clear at present about why so many different approaches are > needed, at the user-interface level, and whether there is anything in > common between them that could usefully be shared within the > implementations in GCC. I'll look at -fbounds-check option. I'll also add some additional description and comparison on GCC Wiki. > > As I understand it, the feature logically splits into: > > * Support the bounds checking within functions but with bounds being lost > for function arguments and return values. This does not need any > architecture-specific support, although it would be slow without such > support. > > * Bounds checking across ABI boundaries. This requires the architecture > support (although one could also imagine such a feature defined in a way > not needing new registers / instructions - again, slow). So, if the user > requests this and doesn't use -mmpx to enable the instructions, an error > would seem appropriate. > > Maybe the first is of too little value without the hardware support (e.g. > too slow), and so the two only make sense together - I don't know. But in > any case it needs to be clear what the machine-independent semantics of > each option are, and what does or does not depend in hardware support. Do > you do anything to disable optimizations that are based on making > deductions from assuming that a pointer is valid? > I've never tried to look at it in such way. I also doubt any of this parts is useful without the other one. Current instrumentation pass is target independent. Implementation allows target dependent support (via implementation of hooks in a target; currently done on i386 only) and general support (via implementation of generic builtin functions and default hooks; currently not implemented). Currently we disable ivopts because we it looses base pointer for memory reference (full optimization disabling is not required here and we are going to turn it on back when find a nice fix). We also have problems with loosing field accesses. I do not think we've found all cases when optimization leads to uncaught bound violations or false bound violations. But we work in this direction and will be glad to recommendations here. > Naming new built-in functions __bnd_* seems unfortunate. We already have > __builtin_*, __sync_* and __atomic_* as built-in function naming > conventions (with consequent checks in various places based on function > name) - do we really need yet another such prefix? > Both __bnd_ and __builtin___bnd variants are available. The reason for builtins name is simple - Intel docs. It is always nice to have compatibility. Actually Intel declares single underscore in the beginning of the name but I could not make it work on C++ with a single underscore. Is there a way to fix it? Thanks, Ilya > -- > Joseph S. Myers > jos...@codesourcery.com
