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. > 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). 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. 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? 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? -- Joseph S. Myers jos...@codesourcery.com
