Re: [gpsd-dev] compile issue: header not necessarily included for atomic_thread_fence

[Gergely Imreh]

On 11/29/2015 07:15 AM, Gary E. Miller wrote:
> Yo Gergely!
> 
> On Thu, 26 Nov 2015 12:17:31 +0800
> Gergely Imreh <address@hidden> wrote:
> 
>>> First thing is we need to know you OS, distro, CC version, etc.  
>>
>> ArchLinux, GCC 5.2.0, scons v2.4.0.rel_2.4.0:3365:9259ea1c13d7.
>> Any other info is needed?
> 
> Intel CPU?  What version of glibc?
> 

Yes, Intel i7 M 620. glibc is at 2.22.


>>> Possibly, but I'd like to know what the __cplusplus is from.  I
>>> have no idea what OS, dirstro, CC would set that and that is way
>>> too generic a test for my taste.  
>>
>> Looking it up, __cplusplus is a standard macro in GCC and "defined
>> when the C++ compiler is in use."
>> https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html
> 
> Right, which is orthongonal to if you system supports stdatomic.h.
> 
> Doing c++ tells you nothing about how your compiler supports atomics.
> 
>> The reason I have that check in the patch, because there's already the
>> exact same check earlier in compilers.h (lines 68-72):
> 
> Yea, you already explained that.  But one piece of bad code is not
> an excuse for a second one.
> 
> If is right to test for C++, but only to include the file as a C
> header and not a C++ header.
> 
> Another test is needed for if you compiler supports stdatomic.h.  That
> is the HAVE_STDATOMIC_H.
> 
> 
>> Because of this, it's possible that the checks evaluate differently,
>> and atomic_thread_fence is called without the header being included.
> 
> Yes, but your patch is to NOT include the header, and we want it included.
> 
>> Hence the patch.
> 
> Hence the need or a good patch.  One that included stdatomic.h in the
> right way, when needed.
> 
> Both where stdatomic.h is included, and where it is used need to
> be on the same page, so they need t both be fixed.
> 
>> That __cplusplus check for the #include was added in commit
>> 79f6d9133378325d70a92e66f7352c1becefbb88 and there's no rationale
>> given in the commit message why it is needed (just a sign-off by you).
> 
> Yeah, shit happens.  Often my own.
> 
> I'm thinking the include part needs to look mmore like this:
> 
> 
>     #ifdef HAVE_STDATOMIC_H
>     #if !defined(__COVERITY__)
>     extern "C" {
>     # endif
> 
>     #include <stdatomic.h>
>     # ifdef __cplusplus
>     }
>     # endif
> 
>     #endif /* __COVERITY__ 
>     #endif /* HAVE_STDATOMIC_H */
> 
> That way if you have stdatomic.h it gets included, in a way that is
> legal for C++.  Then the other place needs to match.
> 

Hm, not quite sure about this. As much as I tried this and checked in
some soruces, extern "C" only works in C++ (thus within a #ifdef
_cplusplus block) since it's not valid C, so the above code would not
compile.

Also looking at some other sources[1], stdatomic.h is not compatible
with C++, where "#include <atomic>" should be used. Not sure if anything
has changed since a middle off 2014 when the linked bug report was written.

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932

Though now it sort of makes sense why to have a check in the first
place. But I don't think the purpose of the fix is to include the header
necessarily, but rather not to call a function that has no header
included... Just a thought.

Cheers,
   Gergely

signature.asc
Description: OpenPGP digital signature