On 24/02/16 23:03, Richard Smith wrote:
On Wed, Feb 24, 2016 at 2:01 PM, Vassil Vassilev <v.g.vassi...@gmail.com> wrote:
On 24/02/16 22:50, Richard Smith wrote:
On Wed, Feb 24, 2016 at 8:10 AM, Vassil Vassilev <v.g.vassi...@gmail.com>
wrote:
On 24/02/16 02:05, Richard Smith wrote:
Calling getMostRecentDecl seems like a slightly fragile way to avoid
iterator invalidation here. Instead...
@@ -214,6 +212,19 @@ namespace clang {
unsigned I = Record.size();
Record.push_back(0);
+ auto &Specializations = Common->Specializations;
+ auto &&PartialSpecializations =
getPartialSpecializations(Common);
+
+ // AddFirstDeclFromEachModule might trigger deserialization,
invalidating
+ // *Specializations iterators. Force the deserialization in
advance.
+ llvm::SmallVector<const Decl*, 16> Specs;
+ for (auto &Entry : Specializations)
+ Specs.push_back(getSpecializationDecl(Entry));
+ for (auto &Entry : PartialSpecializations)
+ Specs.push_back(getSpecializationDecl(Entry));
+ for (auto *D : Specs)
+ D->getMostRecentDecl();
... delete these two lines, and...
+
for (auto &Entry : Specializations) {
... iterate over "Specs" here....
auto *D = getSpecializationDecl(Entry);
... and you don't need this line any more.
assert(D->isCanonicalDecl() && "non-canonical decl in set");
You can then remove the following, identical, PartialSpecializations
loop.
Done.
+ // *Specializations iterators. Force the deserialization in
advance.
Drop the second sentence of this comment, since it's no longer accurate.
oops... thanks!
You also have some tabs in your patch (on the Specs.push_back lines);
Sorry about the tabs, they came from a brand new VM and unconfigured
emacs.
please fix those prior to commit. With those changes, this LGTM.
Would it make sense to ask for commit perms?
Yes, please see the instructions here:
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
Will do. Could you check this patch in meanwhile, please?
r261781. Thanks!
Thanks a lot!
On Mon, Feb 22, 2016 at 7:11 AM, Vassil Vassilev
<v.g.vassi...@gmail.com>
wrote:
ping...
On 30/01/16 21:13, Vassil Vassilev wrote:
On 30/01/16 18:36, David Blaikie wrote:
On Sat, Jan 30, 2016 at 9:00 AM, Vassil Vassilev
<v.g.vassi...@gmail.com>
wrote:
AFAICT the making a test case independent on STL is the hard part. I
think
it will be always failing due to the relocation of the memory region
of
the
underlying SmallVector.
Sorry, I think I didn't explain myself well. What I mean is - due to
the
instability of test cases for UB (especially library UB), we don't
usually
commit test cases for them - we just fix them without tests. About the
only
time I think committing a test is helpful for UB (especially library
UB)
is
if we have a reliable way to test/catch the UB (eg: the sanitizers or a
checking STL that fails fast on validation violations). So, while it
would
be good to provide/demonstrate your test case, I would not commit the
test
case unless it's a minimal reproduction in the presence of one of those
tools (sanitizers or checking STL) - and would just commit the fix
without a
test case, usually.
(I'm not actually reviewing this patch - I don't know much about the
modules
code, but just providing a bit of context and first-pass-review)
Got it. Thanks!
--Vassil
On 30/01/16 17:37, David Blaikie wrote:
Yeah, it's good to have a reproduction, but I wouldn't actually commit
one
- unless you have a checked stl to test against that always fails on
possibly-invalidating sequences of operations, then you'd have a
fairly
simple reproduction
On Jan 30, 2016 8:23 AM, "Vassil Vassilev" <v.g.vassi...@gmail.com>
wrote:
Sorry.
Our module builds choke on merging several modules, containing
declarations from STL (we are using libc++, no modulemaps).
When writing a new module containing the definition of a STL
reverse_iterator, it collects all its template specializations. Some
of
the
specializations need to be deserialized from a third module. This
triggers
an iterator invalidation bug because of the deserialization happening
when
iterating the specializations container itself. This happens only
when
the
container's capacity is exceeded and the relocation invalidates the
iterators and at best causes a crash (see valgrind reports in the bug
report). Unfortunately I haven't been able to make the reproducer
independent on STL.
--Vassil
On 30/01/16 17:08, David Blaikie wrote:
It might be handy to give an overview of the issue in the review (&
certainly in the commit) message rather than only citing the bug
number
On Jan 30, 2016 6:49 AM, "Vassil Vassilev via cfe-commits"
<cfe-commits@lists.llvm.org> wrote:
Attaching a fix to https://llvm.org/bugs/show_bug.cgi?id=26237
Please review.
Many thanks!
--Vassil
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
