Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<algorithm>: fill fails to compile with volatile pointer inputs #1183

Closed
Ryan-rsm-McKenzie opened this issue Aug 12, 2020 · 8 comments · Fixed by #1160
Closed

<algorithm>: fill fails to compile with volatile pointer inputs #1183

Ryan-rsm-McKenzie opened this issue Aug 12, 2020 · 8 comments · Fixed by #1160
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@Ryan-rsm-McKenzie
Copy link

Describe the bug
Passing volatile pointers to fill with arguments that satisfy _Fill_memset_is_safe will fail to compile, as that branch calls memset which cannot take volatile pointers.

Command-line test case

E:\Temp>type repro.cpp
#include <cstdlib>
#include <iostream>
#include <algorithm>

void memzero(void* a_dst, std::size_t a_size)
{
    const auto     beg = static_cast<volatile char*>(a_dst);
    const auto     end = static_cast<volatile char*>(a_dst) + a_size;
    constexpr char val = 0;
    std::fill(beg, end, val);
}

int main()
{
    int num = 42;

    std::cout << num << '\n';
    memzero(&num, sizeof(num));
    std::cout << num << '\n';
}

E:\Temp>cl /EHsc /W4 /WX .\repro.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29115 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.28.29115\include\xutility(4717): error C2664: 'void *memset(void *,int,size_t)': cannot convert argument 1 from '_FwdIt' to 'void *'
        with
        [
            _FwdIt=volatile char *
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.28.29115\include\xutility(4717): note: Conversion loses qualifiers
C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.28.29115\include\vcruntime_string.h(63): note: see declaration of 'memset'
.\repro.cpp(10): note: see reference to function template instantiation 'void std::fill<volatile char*,char>(const _FwdIt,const _FwdIt,const _Ty &)' being compiled
        with
        [
            _FwdIt=volatile char *,
            _Ty=char
        ]

Expected behavior
The program should compile.

STL version

Microsoft Visual Studio Community 2019 Preview
Version 16.8.0 Preview 1.0

Additional context
Both GCC and Clang can compile this example, here's a godbolt link.

It will actually compile if you use std::fill(beg, end, 0); presumably because _Ty is now int and _Fill_memset_is_safe evaluates to false.

@AlexGuteniev
Copy link
Contributor

@AdamBucior , since it overlaps with your PR on review, thoughts?

My understanding is that calling memset is a valid optimization for non-volatile pointers.
But when pointers are volatile, should keep calling loop version.
Should not cast away volatile and call memset, it is not safe.

@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 12, 2020
@BillyONeal
Copy link
Member

We should make this compile but I note that there's nothing useful you get from volatile here as far as the standard is concerned.

@AlexGuteniev
Copy link
Contributor

@BillyONeal , I think it can implement SecureZeroMemory, no?

AdamBucior added a commit to AdamBucior/STL that referenced this issue Aug 12, 2020
@AdamBucior
Copy link
Contributor

Implemented an easy fix in #1160.

@AdamBucior
Copy link
Contributor

Should it have some test coverage?

@AlexGuteniev
Copy link
Contributor

Think so, as this is highly unusual, possibly in a separate test.

@AdamBucior
Copy link
Contributor

Separate test is definitely too much. I would add it to std/tests/VSO_0180469_fill_family if it even needs test coverage.

AdamBucior added a commit to AdamBucior/STL that referenced this issue Aug 12, 2020
@AdamBucior
Copy link
Contributor

Added a simple test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants