On Fri, Jul 19, 2013 at 05:20:39PM +0200, Jakub Jelinek wrote:
> > >I have yet to handle
> > >freeing the hash table, but I think I'll need the GTY machinery for
> > >this (ubsan is not a pass, so I can't just call it at the end of the
> > >pas). Or maybe just create a destructor and use append_to_statement_list.
> >
> > That won't work; append_to_statement_list is for things that happen
> > at runtime, but freeing the hash table is something that needs to
> > happen in the compiler.
>
> Also, I wonder if we need to free the hash table ever, the only meaningful
> place would be when we are done with compilation, if all uses of this (even
> in the future) are from the FEs, perhaps freeing the hash table somewhere
> after parsing of the whole CU finished would be fine, but if we need it even
> from the middle-end, that might be too early.
I'm afraid we'll need it even from the middle-end, at least when doing
the signed overflow sanitization.
> > >+/* This routine returns a magic number for TYPE.
> > >+ ??? This is probably too ugly. Tweak it. */
> > >+
> > >+static unsigned short
> > >+get_tinfo_for_type (tree type)
> >
> > Why map from size to some magic number rather than use the size
> > directly? Also, "tinfo" sounds to me like something to do with C++
> > type_info.
>
> Yeah, get_ubsan_type_info_for_type would be better, plus
> The docs say that it should be log2 of the bit width, so I'd expect that
> you return
> (exact_log2 (TYPE_PRECISION (type)) << 1) | !TYPE_UNSIGNED (type)
> (perhaps with error if exact_log2 returns -1) and thus handle all possible
> type bitsizes. Or if you prefer to use GET_MODE_BITSIZE, that.
Ah, that's much better. I did it like that; also included the
checking of return value of exact_log2. And yeah, the name wasn't
very nice, so I've renamed it.
> Have you checked what clang does if you use typedefs? Does it use
> the names appearing in the typedefs rather than their underlying types,
> or is it TYPE_MAIN_VARIANT (type) actually?
Good point. E.g. for
typedef const unsigned int foo_t;
clang says 'unsigned int'. But we didn't do it; we'd output the foo_t
type. I've adjusted that, so now we print the underlying type as
clang does.
> Also:
>
> ASM_FORMAT_PRIVATE_NAME (tmp_name, ".Lubsan_type", type_var_id_num++);
>
> I don't think the . at the beginning is going to work everywhere
> (on targets that don't support dots in labels),
> I think you want ASM_GENERATE_INTERNAL_LABEL instead and just use
> "Lubsan_type" as the second argument (note, this macro needs a buffer,
> doesn't alloca it).
Okay, fixed. Thanks,
Marek
