search

microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.19k stars 1.5k forks source link

Port DevCom bugs to GitHub #939

Closed StephanTLavavej closed 4 years ago

StephanTLavavej commented 4 years ago

As part of our work to migrate all STL development to GitHub, we're porting our Microsoft-internal bug database ("VSO"; originally Visual Studio Online, renamed Azure DevOps and Azure Boards) to GitHub issues. Some bugs were directly filed in VSO (by STL maintainers and other people within MS, sometimes on behalf of customers), while some were replicated from Developer Community ("DevCom").

As part of this porting process, we aren't resolving any bugs. The intended goal is for GitHub to be the source of truth for all bug reports (in addition to performance/enhancement/etc. suggestions); it's also our preferred place for new issues to be filed. (Issues filed on GitHub aren't replicated anywhere else, so it's the most convenient for us.) However, users who filed DevCom bugs in the past (or continue to file them in the future) will have their bugs remain active, and will receive feedback from STL maintainers there (in addition to their bugs being ported to GitHub issues where they can be linked to pull requests etc.). Additionally, VSO bugs (whether filed directly or replicated from DevCom) will remain active because our bosses and boss-like entities prefer that.

"Porting" a bug to GitHub involves capturing the true essence of the bug report, without distorting or over-simplifying it. (Much like compiler bugs, STL bugs can be very sensitive to the exact code, compiler options, etc. used.) However, we also want our GitHub issues to be readable and comprehensible, so that both maintainers and contributors can understand what's wrong and easily investigate a fix. So, when possible, it's nice to clean up the title and test case, so that they're as clear and minimal as possible. (It is often a good idea to have an "original repro" and "reduced repro", to avoid the dangers of over-simplifying away something important - and I speak as someone who has over-simplified a dozen compiler bug reports in the past.) It's better to err on the side of less cleanup than more - fixing only grammatical issues is fine.

Example bugs ported from DevCom: GH-371 (from DevCom-758960) and GH-503 (from DevCom-371962)

While only the STL maintainers will be able to port the MS-internal bugs that were directly filed in VSO, the DevCom database is publicly viewable, so we could use some help with those bugs. :smile_cat:

Here's a list of the DevCom bugs, along with their VSO IDs and Titles. (Sometimes, multiple DevCom bugs were linked to a single VSO bug, when we had a strong belief that they're all duplicates.) I generated this by hand, so if anything looks wrong (e.g. mismatched VSO/DevCom bugs), please let me know. Note that everyone in this repo has the ability to use our Custom Autolinks in GitHub issues/comments; you can just say DevCom-NNN instead of copying a whole URL.

In general, we have already tried to resolve clearly-invalid bugs, and obvious duplicates, but there are several categories of possible duplicates that we haven't resolved because there may be multiple underlying issues.

Finally, in addition to the title and repro, ported bugs should mention the DevCom and VSO IDs, so we can easily navigate to the linked bugs:

Also tracked by DevCom-publicnumber and VSO-internalnumber / AB#internalnumber.

AB followed by # (not - like other autolinks) will activate automation: your issue will be automatically edited by @msalehmsft to add a hyperlink (it won't appear in a Preview), and the internal bug will gain a special link. This must be mentioned in the original issue to create the internal link; using this syntax in issue comments below will be hyperlinked from GitHub, but not to GitHub.

Please don't use AB#nnn syntax here, in this thread as it will link "Port DevCom bugs to GitHub" to whatever's mentioned. DevCom-nnn and VSO-nnn are safe to mention anywhere.

:beetle: Remaining: VSO ID | DevCom ID(s) | Original VSO Title

:hourglass_flowing_sand: In Progress

:warning: Blocked

:x: Resolved As Invalid

:hammer_and_wrench: Compiler Bug, Reduced To Library-Free Test Case And Sent To Compiler Team

:smile_cat: Fixed

:heavy_check_mark: Done

StephanTLavavej commented 4 years ago

@Amaroker, here's the list as you requested in https://github.com/microsoft/STL/issues/890#issuecomment-649505408. :smiley_cat:

AlexGuteniev commented 4 years ago

Taking the first four:

Returning back, was not able to port:

AlexGuteniev commented 4 years ago

So far the results about first four. Two successes:

Two failures:

Is it good enough?

Amaroker commented 4 years ago

Thanks for the list. I filed a bug report for DevCom-246365 as https://github.com/microsoft/STL/issues/944

There are related issues in the list, for example DevCom-246250 and DevCom-189336. The problem is that they refer to attached code. That code it not publicly available, which makes it difficult.

Amaroker commented 4 years ago

DevCom-334696 has been ported as https://github.com/microsoft/STL/issues/946

AlexGuteniev commented 4 years ago

@StephanTLavavej , I'd suggest that you add checkboxes to your message, so that we copy lines in comment, and you mark checkboxes in your message after an initial check.

AlexGuteniev commented 4 years ago
statementreply commented 4 years ago

Edit: ported as #950, @StephanTLavavej

StephanTLavavej commented 4 years ago

Thanks @AlexGuteniev, @Amaroker, and @statementreply! I really appreciate it! 😺😸 I've updated the table to a checkbox list as requested, and created Remaining/In Progress/Done sections.

If anything simply can't be ported due to non-public attachments (or descriptions that don't make sense, etc.), let us know and we'll handle them separately. I've created a Blocked section for the ones you mentioned, @Amaroker.

AlexGuteniev commented 4 years ago

Ported these:

Did not port:

StephanTLavavej commented 4 years ago

Thanks @AlexGuteniev, I've updated the list again, and @CaseyCarter has linked the MS-internal bugs.

I'm still able to reproduce the DevCom-367683 std::function alignment crash with VS 2019 16.7 Preview 3.1 (note that it is indeed x86 specific and doesn't repro for x64). I'll go ahead and port it.

AlexGuteniev commented 4 years ago

note that it is indeed x86 specific and doesn't repro for x64

Double checked. Yes, probably I tried the wrong configuration.

AlexGuteniev commented 4 years ago
AlexGuteniev commented 4 years ago
AlexGuteniev commented 4 years ago

Ported the following:

Did not port:

statementreply commented 4 years ago

Ported:

Didn't port:

StephanTLavavej commented 4 years ago

Amazing work - I have digested all of your newly-ported bugs, and reorganized the list accordingly (including links to your comments about the bugs that will need special attention). I've also used the "mark comment as resolved" feature to make this thread much easier to scan for pending work.

AlexGuteniev commented 4 years ago
AlexGuteniev commented 4 years ago
StephanTLavavej commented 4 years ago

Thanks! @msalehmsft set up automation that officially links VSO bugs to GitHub, which will enable Microsoft-internal dashboards (I think) and will also make it possible for GitHub PRs to close VSO bugs automatically (*). I've updated all currently ported bugs to use this mechanism.

I'm 80% sure that this can also be activated by contributors. For the next bug that you port, can you phrase the citations as:

"Also tracked by DevCom-publicnumber and VSO-internalnumber / AB#internalnumber."

The exact phrasing doesn't matter (you can say "Originally reported on Developer Community" etc). The new part is saying AB (for Azure Boards, apparently the new name) followed by # (not - like other autolinks) and the internal bug number. (The slash before is unimportant, you could say "and". I've been putting spaces around it out of paranoia.) If this works, your comment will be automatically edited by @msalehmsft to add a hyperlink (it won't appear in a comment Preview), and the internal bug will gain this special link. This must be mentioned in the original issue to create the internal link; using this syntax in issue comments below will be hyperlinked from GitHub, but not to GitHub. (We will still be citing internal bugs as VSO- in code comments etc. hence repeating both forms.)

If this works, I'll update the instructions above.

(*) GitHub PRs will need to say "Fixes AB#nnn" to auto-close bugs (and apparently this accepts "fix", "fixes", "fixed" but not "close"/"resolve").

Amaroker commented 4 years ago

I have ported DevCom-330322 as GH-1033.

AlexGuteniev commented 4 years ago

Did not port:

DevCom bugs are not duplicates, and neither of them is reproducible

DevCom-87221 contains clear repro of the problem that seems to be fixed already, but apparently was present in old Visual Studio:

#include <iostream>
#include <memory>
#include <scoped_allocator>
#include <string>
#include <vector>

#include <cassert>
#include <iostream>
#include <cstddef>
#include <vector>
#include <scoped_allocator>

template <class T>
struct SimpleAllocator
{
    int state;
    using value_type = T;

    //SimpleAllocator() : state(-1)
    //{
    //}
    SimpleAllocator(int state) : state(state)
    {
    }
    SimpleAllocator(const SimpleAllocator& a) : state(a.state)
    {
    }
    template<class U>
    SimpleAllocator(const SimpleAllocator<U>& a) : state(a.state)
    {
    }
    T* allocate(std::size_t n)
    {
        return reinterpret_cast<T*>(malloc(n * sizeof(T)));
    }
    void deallocate(T* p, std::size_t n)
    {
        free(p);
    }
};
template <class T, class U>
bool operator==(const SimpleAllocator<T>& a, const SimpleAllocator<U>& b)
{
    return a.state == b.state;
}
template <class T, class U>
bool operator!=(const SimpleAllocator<T>& a, const SimpleAllocator<U>& b)
{
    return a.state != b.state;
}

int main()
{
    SimpleAllocator<int> a1(1);
    SimpleAllocator<int> a2(2);
    std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>> sa(a1, a2);
    std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>> sa2(sa);
    std::vector<std::vector<int>, std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>>> v(sa2);

    assert(v.get_allocator().inner_allocator() == sa.inner_allocator());
    return 0;
}

The repro does not compile in 19.27.29009.1 It compiles with either changing outer allocator to SimpleAllocator<std::vector<int>>, or with defining #define _ENFORCE_MATCHING_ALLOCATORS 0. In either cases it works.

DevCom-246251 contains clear repro:

#include <iostream>
#include <memory>
#include <scoped_allocator>
#include <string>
#include <vector>

using arena = int;

template<typename T>
struct arena_allocator :std::allocator<T>
{
    using propagate_on_container_copy_assignment = std::false_type;
    using propagate_on_container_move_assignment = std::true_type;
    using propagate_on_container_swap = std::false_type;
    template<typename U>
    struct rebind { using other = arena_allocator<U>; };

    arena* pa;
    arena_allocator() = default;
    arena_allocator(arena* pa) :pa{ pa } {}
    template<typename U>
    arena_allocator(const arena_allocator<U>& x) : pa{ x.pa } {}
};

using string =
std::basic_string<char, std::char_traits<char>, arena_allocator<char>>;
using vector = std::vector<
    string,
    std::scoped_allocator_adaptor<arena_allocator<string>, arena_allocator<char>>
>;

int main()
{
    arena as = 0, ac = 0;

    vector v{
    vector::allocator_type{
    arena_allocator<string>{&as},arena_allocator<char>{&ac}} };

    v.emplace_back("boost");

    std::cout << (v.get_allocator().pa == &as) << "\n";
    std::cout << (v.begin()->get_allocator().pa == &ac) << "\n";
}

Should print

1
1

According to bug report, it prints in 19.11.25331.0 (x86) 

1
0

According to bug report, in 19.00.23026 it prints the expected output.

So, I've tried 19.26.28806 and it prints the correct results. Possible reasons:

DevCom-246246 contains a repro is only slightly different from DevCom-87221 . The main difference part is that line from DevCom-246246:

    std::vector<int, std::scoped_allocator_adaptor<SimpleAllocator<int>>> v(sa2);

Is replaced with line from DevCom-87221:

    std::vector<std::vector<int>, std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>>> v(sa2);

(thanks @Amaroker for pointing that out)

So, the DevCom-246246 reporduced, but it does no look like a valid bug.

AlexGuteniev commented 4 years ago

Ported:

Amaroker commented 4 years ago

@AlexGuteniev

The repro code that you showed above for DevCom-246246 seems to actually belong to DevCom-246251. Can you double check, did you mix them up? The titles are very similar.

The repro of DevCom-246246 causes an assertion failure.

AlexGuteniev commented 4 years ago

Can you double check, did you mix them up? The titles are very similar.

I've checked, Yes, almost duplicate title, plus code of DevCom-246246 and DevCom-87221 is similar. Still I cannot make a valid bug of these already three different repros.

CaseyCarter commented 4 years ago

The repro of DevCom-246246 causes an assertion failure.

The repro is invalid. This assertion:

std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>> sa2(sa);
std::vector<int, std::scoped_allocator_adaptor<SimpleAllocator<int>>> v(sa2);

assert(v.get_allocator().inner_allocator().i == sa.inner_allocator().i);

should fire on a correct implementation. The scoped_allocator_adaptor used with vector has only one allocator type, so get_allocator().inner_allocator() returns a copy of get_allocator(). The assert passes if changed to:

assert(v.get_allocator().inner_allocator().i == 1);

or:

assert(v.get_allocator().inner_allocator().inner_allocator().inner_allocator().inner_allocator().i == 1);

The repro also works if we change the vector specialization to be what the submitter intended:

std::scoped_allocator_adaptor<SimpleAllocator<int>, SimpleAllocator<int>> sa2(sa);
std::vector<int, decltype(sa)> v(sa2);

assert(v.get_allocator().inner_allocator().i == sa.inner_allocator().i);

I can believe there was a bug here in 19.11, but it seems to have been fixed by @BillyONeal's work in <scoped_allocator> in that era. The copy constructor was working by 19.14, at least: https://godbolt.org/z/75Kr3v.

Did not port:

DevCom bugs are not duplicates, and neither of them is reproducible

Thanks for the investigation, @AlexGuteniev. I'll close these as fixed.

AlexGuteniev commented 4 years ago

I was able to port DevCom-77779 and DevCom-246249 , see https://github.com/microsoft/STL/issues/939#issuecomment-657100932

Originally I noted that DevCom-246249 does not have public attachment, but DevCom-77779 has an attachment in its comment, which is clearly that missing atttachment.

AlexGuteniev commented 4 years ago

(It would be more convenient to move already fixed or invalid bugs to yet another section of this issue instead of striking. Or in the same section with ported. Negative result is also a result)

AlexGuteniev commented 4 years ago

Porting:

(duplicates)

AlexGuteniev commented 4 years ago

Suspicious. @StephanTLavavej , please take a look: I've ported DevCom-657074 as GH-1037 , but the error is not C2664, and it has nothing to do with std::exception_ptr.

AlexGuteniev commented 4 years ago
AlexGuteniev commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
StephanTLavavej commented 4 years ago

Thanks @Amaroker, @AlexGuteniev, and @fsb4000! (And @CaseyCarter for resolving bugs!) I've updated the (rapidly shrinking :heart_eyes_cat:) main list, with separate sections for invalid and fixed bugs. I've labeled all of your amazingly-ported bugs and made sure they're linked up on the MS side.

@AlexGuteniev - I looked at the GH-1037 xtree swap bug, and I believe that the error C2664 complaining about exception_ptr vanished when we changed exception_ptr's swap to be a hidden friend. (That was Microsoft-internal MSVC-PR-197905 which @BillyONeal merged on Aug 20, 2019, immediately before the beginning of GitHub history.) So it's nothing to be concerned about, and you've accurately captured the issue. Thanks for double-checking!

fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
StephanTLavavej commented 4 years ago

Thanks @fsb4000! I've labeled, linked up, and recorded all of your ported bugs. Extra thanks for verifying the already-fixed UWP filesystem bug, and finding the get_time duplicate.

By the way, note that you can take advantage of GitHub autolinking in this repo, you don't need to write links manually. (GH-number works identically to #number in all repos; we've added the DevCom-number and VSO-number syntax, which is a feature available to Pro/Enterprise repos.)

fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago
StephanTLavavej commented 4 years ago

Thanks again @fsb4000! I've labeled and linked up all of those bugs.

I've also added 3 new DevCom bugs to the list; with them, there are only 9 Remaining and 4 Blocked! :smiley_cat:

AlexGuteniev commented 4 years ago

This one appears to be compiler issue. Test case without std::thread

a.cpp:

#include "c.h"

#include <iostream>

static void f()
{
    invoke([&]() { std::cout << "lambda1\n"; })();
}

void g();

int main()
{
    f();
    g();
}

b.cpp:

#include "c.h"

#include <iostream>

static void f ()
{
  invoke([&]() { std::cout << "lambda2\n"; })();
}

void g () { f (); }

c.h:

#pragma once

template<class F>
void invoke_impl(void* f) {
    auto fn = static_cast<F*>(f);
    (*fn)();
}

template<class F>
struct invoke
{
    invoke(F f)
    {
        fn = &invoke_impl<F>;
        context = &f;
    }

    void operator()() { fn(context); }

    void (*fn)(void*);
    void* context;
};
fsb4000 commented 4 years ago
fsb4000 commented 4 years ago

This one appears to be compiler issue.

And it seems to be regression: Visual Studio 2019 16.7 prints: 

lambda2
lambda2

But Visual Studio 2017 (15.9.25) prints:

lambda1
lambda2