[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [gpsd-dev] compile issue: header not necessarily included for atomic
From: |
Gergely Imreh |
Subject: |
Re: [gpsd-dev] compile issue: header not necessarily included for atomic_thread_fence |
Date: |
Sun, 29 Nov 2015 17:30:49 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
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