is broken, while libstdc++ supports it.
struct A {
deque<A> q;
};
I regard this as a QoI issue, and a somewhat important one.
1. vector, map support it; 2. no workaround unless discarding
user's intended API.
An simple fix is to replace the constant __block_size with
a functor (delay the use of sizeof()), then compiler will optimize
the function call out -- this works for c++03 as well, but results
in ABI breakage, while I see no fix involves no ABI change.
This is not allowed by the standard, why would you want to promote using undefined features? Better get libstdc++ to conform. 
-Dimitry
Hello,
is broken, while libstdc++ supports it.
struct A {
deque<A> q;
};
Related to this is this (fixed) bug: <http://llvm.org/bugs/show_bug.cgi?id=9351> with
a discussion about incomplete types in standard containers. This comment also seems
relevant: <http://llvm.org/bugs/show_bug.cgi?id=13471#c3>
HTH,
Jonathan
AFAIK, the standard said nothing about whether and when to allow it,
that does not mean the standard do not allow it. This is a difference:
if an implementation allows it, it's not non-confirming, and user can
benefit from it, and we already have those users, as reported to
FreeBSD -- the "pan" news client. Please show me where the standard
does not allow it (which means, an implementation allows it is
non-confirming).
is broken, while libstdc++ supports it.
struct A {
deque<A> q;
};I regard this as a QoI issue, and a somewhat important one.
1. vector, map support it; 2. no workaround unless discarding
user's intended API.This is not allowed by the standard, why would you want to promote using undefined features? Better get libstdc++ to conform.
AFAIK, the standard said nothing about whether and when to allow it,
that does not mean the standard do not allow it.
The standard says it is undefined (ergo, don't rely on it). Quoting the C++11 standard, part 17.6.4.8, paragraph 2:
2) In particular, the effects are undefined in the following cases:
[ ... rest of list elided ...]
— if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component.
In previous standards, the "unless specifically allowed for that component" part was not even there. But none of the components I could find for which it *is* specifically allowed in C++11 are containers:
20.2.4 Function template declval
20.7.1 Class template unique_ptr
20.7.1.1 Default deleters
20.7.2.2 Class template shared_ptr
20.7.2.3 Class template weak_ptr
20.7.2.4 Class template enable_shared_from_this
See also this Dr Dobbs article from 2002 (!) for some more in-depth information:
This is a difference:
if an implementation allows it, it's not non-confirming, and user can
benefit from it, and we already have those users, as reported to
FreeBSD -- the "pan" news client.
The pan program is actually one of the first of many C++ programs in FreeBSD's ports collection that encountered this specific problem, so I don't think this is enough reason to change libc++'s internal implementation details just for it. (Note that we can currently compile Qt, almost all of KDE and Libre/OpenOffice with clang and libc++, which are C++ codebases much more complicated and extensive than a news reader.)
My opinion is that the pan developers are using the C++ standard library incorrectly, and the argument "but it works with gcc" is simply rather weak.
That said, I see no reason that pan can't be fixed. It should not need any dramatic changes.
-Dimitry
I don't really remember many other issues in pkgsrc either and most of
the libc++ specific issues are fixed. news/pan ended up being backed
out, it was nast to do...
Joerg
2) In particular, the effects are undefined in the following cases:
Of course it's undefined, but implement something in a defined way
does not make an implementation non-confirming. Otherwise, all
compilers with well-defined signed overflow behavior are non-confirming.
The pan program is actually one of the first of many C++ programs in FreeBSD's ports collection that encountered this specific problem, so I don't think this is enough reason to change libc++'s internal implementation details just for it.
Two months ago one of my crappy program ran into a linker issue with
libstdc++'s __normal_iterator's operator+. Of course standard says nothing
about whether implementation defined iterator types' operator+ can or can not
odr-use its argument, but they still fixed this. What does that mean?
Improving QoI is nothing shame. No matter how many user can benefit from
it, placing unneeded barrier on harmless issue should not be recommended.
My opinion is that the pan developers are using the C++ standard library incorrectly, and the argument "but it works with gcc" is simply rather weak.
libstdc++ regards failing to support such use as *regressions* -- this
is a feature, and can be safely addressed without people propose it.
That said, I see no reason that pan can't be fixed. It should not need any dramatic changes.
No matter you see an reason or not, an issue like this has been
identified as bug and being *fixed* before:
Hi all,
Clang may reject using a PCH file for various reasons, e.g. because it was created from a different clang version, because an include file was modified, etc.
I propose that we add an option like '--verify-pch /path/to/pch/file' in clang that when invoked will:
-Load the given PCH file
-Verify it (check compiler version, stat includes, etc.)
-Return 0 on success or non-zero and a diagnostic error on failure.
This can be used by a build system, if it so chooses, as part of its build dependencies calculation in order to verify that a PCH file is usable, and rebuild it if it is not.
Clang is the "ultimate authority" on what constitutes a usable PCH file, so it makes sense to delegate to it.
Apart from making the experience better when the PCH file becomes stale in ways the build system does not detect (e.g. due to different compiler version), another advantage of this approach is to allow us to reduce the amount of PCH verification that clang does by only doing it once at the beginning of the build process.
Some time ago we stopped stat'ing the system headers during PCH verification, since we considered it not worth the cost to stat them for every clang invocation. If the build system checked once at the beginning and then informed clang on subsequent invocations that it doesn't need to re-verify the PCH again, we could bring back verification of system headers.
Let me know what you think.
-Argyrios
2) In particular, the effects are undefined in the following cases:
Of course it's undefined, but implement something in a defined way
does not make an implementation non-confirming. Otherwise, all
compilers with well-defined signed overflow behavior are non-confirming.
Sure, but then you are not writing standards-based C++ anymore. Your
code would only work on specific implementations, which happen to
implement the behavior you expect.
The pan program is actually one of the first of many C++ programs in FreeBSD's ports collection that encountered this specific problem, so I don't think this is enough reason to change libc++'s internal implementation details just for it.
Two months ago one of my crappy program ran into a linker issue with
libstdc++'s __normal_iterator's operator+. Of course standard says nothing
about whether implementation defined iterator types' operator+ can or can not
odr-use its argument, but they still fixed this. What does that mean?
That means that libstdc++ authors are flexible, which is nice of them,
and are happy to implement the behavior *you* expect. Now there might
be a problem when somebody else expects something different.
Improving QoI is nothing shame. No matter how many user can benefit from
it, placing unneeded barrier on harmless issue should not be recommended.
...
No matter you see an reason or not, an issue like this has been
identified as bug and being *fixed* before:
Well, if this can be fixed in a similar way, and Howard thinks it is
worthwhile, I don't see why not. But personally I would still refrain
from relying on it...
-Dimitry
Wouldn’t that mean that the build system had to launch clang for every pch file in noop builds? And don’t you want to force a full rebuild on compiler version changes anyhow?
Wouldn’t that mean that the build system had to launch clang for every pch file in noop builds?
Not necessarily, It would definitely have to launch it if there is going to be a clang invocation using the PCH file.
And don’t you want to force a full rebuild on compiler version changes anyhow?
Again, not necessarily, incremental version changes are not supposed to change the semantics of your code. And the version is but one of multiple requirements for PCH verification.
Now there might
be a problem when somebody else expects something different.
So we only fix the cases without this possibility
Well, if this can be fixed in a similar way, and Howard thinks it is
worthwhile, I don't see why not. But personally I would still refrain
from relying on it...
If more people rely on it, it's gonna be a good reason to
standardize it
Here is my proposed change: remove the _BlockSize template
parameter from __deque_iterator. This does not decrease the
identify of __deque_iterator since it's templated on _ValueType
which covers the_BlockSize difference already.
Patch is attached. Tests (+ a new one) passed.
deque_rec.patch (58.5 KB)
Discussion of a similar patch, which was rejected due to ABI concerns:
http://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg80909.html
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-October/032316.html
If you would like to make this change I would suggest doing it via an ABI
migration.
Thanks,
If the library working group of the C++ committee decided that the standard should allow this (because, say,
if someone filed a defect at C++ Standard Library Active Issues List against the standard), then libc++ would be required to support this.
Also, I would suggest opening a bug at llvm.org/bugs, and attaching one of these patches (they look remarkably similar).
That won't get it applied sooner, but at least it will be hanging around the bug system, and someone will remember this conversation
the next time the subject comes up (I certainly did not!) and we can avoid having the same conversations over and over.
-- Marshall
Marshall Clow Idio Software <mailto:mclow.lists@gmail.com>
A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
-- Yu Suzuki
Discussion of a similar patch, which was rejected due to ABI concerns:
Re: [PATCH] [libc++] Make std::deque instantiable with incomplete element type
This patch seems has a tiny(?) issue: _LIBCPP_CONSTEXPR
does not fallbacks to inline (constexpr does inline by default),
so if it's not defined, then `__deque_block_size` is going to
run into an odr violation.
BTW, LIBCPP_CONSTEXPR usage on __block_size is
also unneeded.
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-October/032316.html
If you would like to make this change I would suggest doing it via an ABI
migration.
Well...
BTW, I tried to workaround this issue by detecting incomplete
type. And, as you can think of, I found no way to escape from an
odr violation.
Wouldn't that mean that the build system had to launch clang for every pch
file in noop builds?Not necessarily, It would definitely have to launch it if there is going
to be a clang invocation using the PCH file.And don't you want to force a full rebuild on compiler version changes
anyhow?Again, not necessarily, incremental version changes are not supposed to
change the semantics of your code.
Sure, it's not 100% necessary, but it's a good idea and should always be
done in practice (say, if you track performance over time, you don't want
half of your .o files built with an older compiler and half with a newer),
right?
I can think of two things causing a PCH file to be invalid:
1. It's invalid due to one of its dependency inputs (the compiler binary
that generated it, the .h files it includes) changing. Build systems
rebuild files whose dependency inputs change, and the compiler probably
always writes valid PCH files (if it doesn't, that's just a bug in the
compiler, and if the compiler has such a bug, regenerating the PCH file
again will probably just write an invalid PCH file again).
2. It's invalid due to interaction with the translation unit it's included
from. This probably can't happen (maybe the PCH uses some module include
that's not compatible with some other module used by the translation
unit?), but if it does it can't be diagnosed by a standalone diagnostic
pass (since that doesn't check the PCH file in interaction with other PCH
files).
Did I miss a scenario, or are you worried about scenario 1 and your build
system just doesn't track all dependencies for PCH files?
Is there a build system that add the compiler binary as dependency for all compilers filed by default ?
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
– Jean-Daniel
I did a quick check and it looks like ninja (on my llvm build) doesn’t rebuild if the compiler changes; and xcodebuild doesn’t either.
Yes, the build system does not track all dependencies for PCH files.
Apart from the compiler version, neither ninja (at least in my llvm build) nor xcodebuild track system headers.
There is also a PCH dependency that is not even reported by clang. If you import a modularized header in modules-mode (e.g “#import <Foundation/Foundation.h>”) clang should still report the headers as dependencies but a module file will also be created in the module cache that the PCH will point to. Now if you delete the module cache then the PCH is unusable.
The last one is a somewhat corner case, but it illustrates that delegating to clang is a good “catch-all” to check if the PCH needs rebuilding or not.
-Argyrios
Wouldn't that mean that the build system had to launch clang for every
pch file in noop builds?Not necessarily, It would definitely have to launch it if there is going
to be a clang invocation using the PCH file.And don't you want to force a full rebuild on compiler version changes
anyhow?Again, not necessarily, incremental version changes are not supposed to
change the semantics of your code.Sure, it's not 100% necessary, but it's a good idea and should always be
done in practice (say, if you track performance over time, you don't want
half of your .o files built with an older compiler and half with a newer),
right?I did a quick check and it looks like ninja (on my llvm build) doesn't
rebuild if the compiler changes; and xcodebuild doesn't either.
Ninja does what you tell it to do. You can certainly make gch files depend
on the compiler if you want. You are in a position to change xcodebuild to
do this too, I believe
I can think of two things causing a PCH file to be invalid:
1. It's invalid due to one of its dependency inputs (the compiler binary
that generated it, the .h files it includes) changing. Build systems
rebuild files whose dependency inputs change, and the compiler probably
always writes valid PCH files (if it doesn't, that's just a bug in the
compiler, and if the compiler has such a bug, regenerating the PCH file
again will probably just write an invalid PCH file again).2. It's invalid due to interaction with the translation unit it's included
from. This probably can't happen (maybe the PCH uses some module include
that's not compatible with some other module used by the translation
unit?), but if it does it can't be diagnosed by a standalone diagnostic
pass (since that doesn't check the PCH file in interaction with other PCH
files).Did I miss a scenario, or are you worried about scenario 1 and your build
system just doesn't track all dependencies for PCH files?Yes, the build system does not track all dependencies for PCH files.
Apart from the compiler version, neither ninja (at least in my llvm build)
nor xcodebuild track system headers.
Ninja's philosophy that it's up to the generator to think of things like
this. In Chromium, we considered giving gch files a dependency on the
compiler, but ended up just deleting the build directory on clang updates
as that was easier to do and was Good Enough for us.
There is also a PCH dependency that is not even reported by clang. If you
import a modularized header in modules-mode (e.g "#import
<Foundation/Foundation.h>") clang should still report the headers as
dependencies but a module file will also be created in the module cache
that the PCH will point to. Now if you delete the module cache then the PCH
is unusable.
Is that an issue for general dependency tracking too? Should the module
cache be reported in the -MD output?
Sure, flexibility is paramount. Noone is going to be forced to use this new flag, do you have specific concerns that make you opposed to this flag getting in ?
I prefer not. The module cache should not be leaking out, the whole mechanism is designed to be transparent, the dependencies reported should be the header files and the module.map if one exists (there’s a bug related to modules and dependencies list but this is another issue…)