Want input to solve PR18323

Hi all,

The file Headers/emmintrin.h contains macros where variables are
created inside the macro definition which can cause name collisions
and PR18323 shows an example where this has happened. Variables were
added to macros in R143792 with comment "Fix vector macros to
correctly check argument types. <rdar://problem/10261670>".

One way of solving this particular PR is to revert R143792, but since
that has led to other problems according to the commit message above
I'm not sure if that's the right way and would appreciate other
suggestions or comments.

/ David

So the preprocessed code looks like:
__m128i __a = …

extension ({
__m128i __a = (__a);
(__m128i)__builtin_ia32_pslldqi128(__a, (count)*8); })

… making __a uninitialized. Is that correct?

Can you pre-process the source code and upload that to the PR?

I would say that the user is using a double-underscore name which is in the implementers namespace and they shouldn’t do that. We can probably go ahead and change the name in this instance just to fix the gcc test suite.

So the preprocessed code looks like:
__m128i __a = ...
__extension__ ({
  __m128i __a = (__a);
  (__m128i)__builtin_ia32_pslldqi128(__a, (count)*8); })

... making __a uninitialized. Is that correct?

I think so, yes.

Can you pre-process the source code and upload that to the PR?

Done

I would say that the user is using a double-underscore name which is in the
implementers namespace and they shouldn't do that. We can probably go ahead
and change the name in this instance just to fix the gcc test suite.

The problem is that the name isn't coming from the test suite but from
the libstdc++ implementation of <random> (see the PR for details). As
such I guess there will be more of a discussing regarding who can use
that namespace. What I don't like about just changing the variable
name in the macro is that there is no guarantee that a similar name
clash won't happen again. There's also no warning produced when
compiling since these are considered system headers which make the
problem hard to find.

Interesting. Here’s the libstdc++ code that uses __a:
http://gcc.gnu.org/onlinedocs/gcc-4.8.0/libstdc++/api/a01396_source.html#l00055

Ted Kremenek added pragmas that silence warnings about this in October:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/emmintrin.h?r1=192143&r2=192142&pathrev=192143

Interesting. Here's the libstdc++ code that uses __a:
http://gcc.gnu.org/onlinedocs/gcc-4.8.0/libstdc++/api/a01396_source.html#l00055

Ted Kremenek added pragmas that silence warnings about this in October:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/emmintrin.h?r1=192143&r2=192142&pathrev=192143

The only way I can think of to fix this with the type checking is to use a
helper like:

static inline always_inline,nodebug __m128i __check_type_m128i(__m128i __a)
{ return __a; }
#define _mm_slli_si128(a, __sl2) \
(__m128i)__builtin_ia32_pslldqi128(__check_type_m128i(a), (count)*8)

Thanks for the suggestion. I tested it and it seems to work
(check-clang and test-case in PR passes). There are quite a few macros
in the lib\Header files which have potential for this kind of
name-clash (>100). I think it's a good idea to fix them all but I
would like some reassurance that this is the correct path before
attempting this. Are there any more tests that I should perform? Where
should these kind of helper functions be placed since they are needed
in multiple intrinsic files?

This approach has the downside that we're injecting a whole bunch of names
into the global namespace, which is annoying. We still have to hope the
user doesn't shadow the function name. It might be better to make the
local variable name longer and more obscure.