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

Turn off BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS for MSVC++ #57

Merged
merged 2 commits into from
Apr 20, 2018
Merged

Turn off BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS for MSVC++ #57

merged 2 commits into from
Apr 20, 2018

Conversation

BillyONeal
Copy link
Contributor

Boost is attempting to use a function not in the standard, seekpos(),
guarded by this setting. Looks like Boost can use the standards
conforming interface for MSVC++ instead. It should work going
back effectively "forever" as far as MSVC++ is concerned, but this
guard applies the change only to 2017. It's possible there is a better
control in boost.config I missed.

Boost is attempting to use a function not in the standard, seekpos(),
guarded by this setting. Looks like Boost can use the standards
conforming interface for MSVC++ instead. It should work going
back effectively "forever" as far as MSVC++ is concerned, but this
guard applies the change only to 2017. It's possible there is a better
control in boost.config I missed.
@eldiener
Copy link
Contributor

The question is how we should be targeting vc++ 14.1 ? I think we should use BOOST_MSVC from config, which holds the value of vc++'s _MSC_VER. Using a recent macro to identify a version of the vc++'s standard library seems very chancy to me.

@BillyONeal
Copy link
Contributor Author

_MSC_VER should be OK if you'd prefer it; we added _MSVC_STL_VERSION specifically at the request of Boost for separate version numbers between the compiler and library (as sometimes new compilers get used with old libraries).

@eldiener
Copy link
Contributor

eldiener commented Apr 18, 2018

I would not use _MSC_VER directly in your PR but specifically BOOST_MSVC. This is because some other compilers, which are not vc++, also define _MSC_VER whereas BOOST_MSVC means that vc++ specifically is being used. Also are you really saying that newer versions of vc++ could be used with old versions of your C++ standard library ? I did not think that was possible in Microsoft distributions.

When you say that 'Boost can use the standards conforming interface for MSVC++ instead. It should work going back effectively "forever" as far as MSVC++ is concerned' do you mean that all older versions of VC++, with their distributed versions of the C++ standard library, have a standards conforming interface as far as this problem is concerned ? That would be strange as it would put in question the original reason for the vc++ specific code in this particular situation.

@BillyONeal
Copy link
Contributor Author

BillyONeal commented Apr 18, 2018

Also are you really saying that newer versions of vc++ could be used with old versions of your C++ standard library ?

Yes, its a thing people do sometimes. Old libs with new compiler should work, old compiler with new libs is unsupported.

do you mean that all older versions of VC++, with their distributed versions of the C++ standard library, have a standards conforming interface as far as this problem is concerned ?

Modulo bugs, yes. I only tested with 2017 though which is why I so guarded it that way.

That would be strange as it would put in question the original reason for the vc++ specific code in this particular situation.

The Dinkumware code path is used for other Dinkumware licensees; they have this behavior of "call fsetpos followed by fseek" because the C standard says it's UB to call fsetpos on anything not returned by a call to fgetpos. (See C11, 7.21.9.3 "The fsetpos function"/2) But Dinkumware's caution there happens to be unnecessary for at least our current CRT implementation, where fsetpos just sets a 64 bit offset.

There are some bugs that happened with VS 2017 15.7 and earlier where directly comparing fpos instances with non-streamoff integer types created ambiguities (compiler didn't know whether to convert fpos into streamoff and widen other integer, or convert other integer into fpos and compare those) which I fixed in 15.8 by adding comparison operators between fpos and all other integral types. (Note that 15.8 isn't even in preview yet; I just fixed this in our sources last week and found out about Boost depending on this nonstandard seekoff() member I removed because we build Boost with the prerelease compiler every night)

@BillyONeal
Copy link
Contributor Author

directly comparing fpos instances with non-streamoff integer types

Also note that the non-Dinkumware path in Boost already deals with this problem by directly casting the fpos to streamoff first.

@eldiener
Copy link
Contributor

eldiener commented Apr 19, 2018

The original code path is for Dinkumware rather than for vc++ specifically. If you are saying that vc++ always worked correctly with Dinkumware and did not need this one-off code path we could remove it for any version of vc++ by just checking if BOOST_MSVC is defined. But to be more conservative than that, in order not to destroy the use of iostreams for past versions of vc++ other than the present one, I am fine with approving your PR as long as _MSVC_STL_VERSION is never used by any other compiler, which I assume and hope will be the case, without checking for BOOST_MSVC. To be really conservative though it might be best to check for both _MSVC_STL_VERSION and BOOST_MSVC as a reason to not use the code path. Can you change it appropriately to do that or do you want me to merge the PR and do it myself ? BOOST_MSVC is defined whenever vc++ is the compiler and not defined otherwise.

@BillyONeal
Copy link
Contributor Author

I was just informed last night that there's a Boost.IOStreams test failing right now; let me investigate and I'll keep you posted. Thanks!

@BillyONeal
Copy link
Contributor Author

Turns out the test failure is because I broke basic_stringbuf::seekoff last week :/, so not related to this change. I'll make the change you requested to test BOOST_MSVC first as soon as I hear I haven't broken boost.iostream any more. :)

@BillyONeal
Copy link
Contributor Author

OK, confirmed that it's just because I made seekoff corrupt basic_stringbuf which I just fixed. (Yay, thank you boost iostreams tests for catching my mistake!) Updated PR with the additional test you requested.

@MarcelRaad
Copy link
Contributor

Could this be merged to master please? This is needed for VS 2017 Update 8 as of Preview 3.0.

@garethsb
Copy link

garethsb commented Jul 9, 2018

As the original author of the workarounds in boost/iostreams/positioning.hpp, I can confirm they absolutely were necessary back when I produced them in 2005, but that's a long time ago now, even for Boost. ;-)

@BillyONeal
Copy link
Contributor Author

I apologize on behalf of my predecessors? :P

@alanbirtles
Copy link

No so long ago, this was only fixed in VS 2012, see https://groups.google.com/forum/#!searchin/boost-developers-archive/_FPOSOFF%7Csort:date/boost-developers-archive/VT7UxdJwfn4/MXpi6gyoBAAJ for some more discussion

@BillyONeal
Copy link
Contributor Author

Wait wait wait wait we cast it to long? WHY?!???! :(

@alanbirtles
Copy link

Before VS2010 lots of ugly hacks were required for seeking past 2GB in std::fstream for similar reasons.

@mnordie
Copy link

mnordie commented Oct 16, 2018

Any idea when this will be released?
Doesn't seem to be in 1.68.

@jeking3
Copy link
Contributor

jeking3 commented Oct 16, 2018

This should appear in 1.69.0 - I think it was supposed to be in 1.68.0 but wasn't merged in time. It is in the pipeline now.

@mwpowellhtx
Copy link

I'm receiving the same error in iostreams/positioning.hpp. Make sure y'all check and double check your regression before making that commit and/or merging, please.

@BillyONeal
Copy link
Contributor Author

@mwpowellhtx That should be guarded by the ifdef here?

# ifndef BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS

@mwpowellhtx
Copy link

@BillyONeal It's a good question, it is, but how do I know whether I have "Dinkumware fpos"?

Meanwhile, I am defining _SILENCE_FPOS_SEEKPOS_DEPRECATION_WARNING, but letting the deprecated (returning zero) seekpos() linger just seems like asking for regression trouble.

@BillyONeal
Copy link
Contributor Author

@mwpowellhtx It would be more appropriate to call it "workaround broken Dinkumware behavior where they put file sizes into an int" :)

@mwpowellhtx
Copy link

@BillyONeal Copy that; I understand. 👍

I installed the patch, BTW, and it appears to be working. I still have to "acknowledge" the issue, so to speak, but at least I can proceed forward.

@K-M-Ibrahim-Khalilullah
Copy link

@BillyONeal
would tell me please how can I turn off Boost_IOstream_has_dinkkumware_fpos? I already install boost with pcl!

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Apr 18, 2024
Boost stopped using `_FPOSOFF` when boostorg/iostreams@7c2592c was merged on 2016-09-16. I've verified that this shipped in Boost 1.63.0 on 2016-12-26.

We deprecated `std::fpos::seekpos()` with MSVC-PR-132953 merged on 2018-07-18. (We first noticed the problem when MSVC-PR-115404 was merged on 2018-04-11.) Boost stopped using it when boostorg/iostreams#57 was merged on 2018-04-20. I've verified that this shipped in Boost 1.69.0 on 2018-12-12.

Note that while the `std::fpos` type appears in the parameter types and return types of dllexported functions, `std::fpos` is not dllexported itself, as indicated by the lack of explicit calling conventions in the source. Because `std::fpos::seekpos()` isn't dllexported, we can freely remove it. (The only mentions of `seekpos` in the dllexport surface are for the different `std::basic_streambuf::seekpos()`, which conveniently also demonstrates how it takes and returns `std::fpos`.)

```
D:\GitHub\STL\out\x64\out\bin\amd64>dumpbin /exports msvcp140d_oss.dll | rg "\bseekpos\b"
       1206  4B5 00053D70 ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<char,struct std::char_traits<char> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1207  4B6 00053DB0 ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<unsigned short,struct std::char_traits<unsigned short> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1208  4B7 00053DF0 ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<wchar_t,struct std::char_traits<wchar_t> >::seekpos(class std::fpos<struct _Mbstatet>,int))
```
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Apr 19, 2024
Boost stopped using `_FPOSOFF` when boostorg/iostreams@7c2592c was merged on 2016-09-16. I've verified that this shipped in Boost 1.63.0 on 2016-12-26.

We deprecated `std::fpos::seekpos()` with MSVC-PR-132953 merged on 2018-07-18. (We first noticed the problem when MSVC-PR-115404 was merged on 2018-04-11.) Boost stopped using it when boostorg/iostreams#57 was merged on 2018-04-20. I've verified that this shipped in Boost 1.69.0 on 2018-12-12.

Note that while the `std::fpos` type appears in the parameter types and return types of dllexported functions, `std::fpos` is not dllexported itself, as indicated by the lack of explicit calling conventions in the source. Because `std::fpos::seekpos()` isn't dllexported, we can freely remove it. (The only mentions of `seekpos` in the dllexport surface are for the different `std::basic_streambuf::seekpos()`, which conveniently also demonstrates how it takes and returns `std::fpos`.)

```
D:\GitHub\STL\out\x64\out\bin\amd64>dumpbin /exports msvcp140d_oss.dll | rg "\bseekpos\b"
       1206  4B5 00053D70 ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<char,struct std::char_traits<char> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1207  4B6 00053DB0 ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<unsigned short,struct std::char_traits<unsigned short> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1208  4B7 00053DF0 ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<wchar_t,struct std::char_traits<wchar_t> >::seekpos(class std::fpos<struct _Mbstatet>,int))
```
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Apr 19, 2024
Boost stopped using `_FPOSOFF` when boostorg/iostreams@7c2592c was merged on 2016-09-16. I've verified that this shipped in Boost 1.63.0 on 2016-12-26.

We deprecated `std::fpos::seekpos()` with MSVC-PR-132953 merged on 2018-07-18. (We first noticed the problem when MSVC-PR-115404 was merged on 2018-04-11.) Boost stopped using it when boostorg/iostreams#57 was merged on 2018-04-20. I've verified that this shipped in Boost 1.69.0 on 2018-12-12.

Note that while the `std::fpos` type appears in the parameter types and return types of dllexported functions, `std::fpos` is not dllexported itself, as indicated by the lack of explicit calling conventions in the source. Because `std::fpos::seekpos()` isn't dllexported, we can freely remove it. (The only mentions of `seekpos` in the dllexport surface are for the different `std::basic_streambuf::seekpos()`, which conveniently also demonstrates how it takes and returns `std::fpos`.)

```
D:\GitHub\STL\out\x64\out\bin\amd64>dumpbin /exports msvcp140d_oss.dll | rg "\bseekpos\b"
       1206  4B5 00053D70 ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@DU?$char_traits@D@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<char,struct std::char_traits<char> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1207  4B6 00053DB0 ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@GU?$char_traits@G@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<unsigned short,struct std::char_traits<unsigned short> >::seekpos(class std::fpos<struct _Mbstatet>,int))
       1208  4B7 00053DF0 ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z = ?seekpos@?$basic_streambuf@_WU?$char_traits@_W@std@@@std@@Meaa?AV?$fpos@U_Mbstatet@@@2@V32@H@Z (protected: virtual class std::fpos<struct _Mbstatet> __cdecl std::basic_streambuf<wchar_t,struct std::char_traits<wchar_t> >::seekpos(class std::fpos<struct _Mbstatet>,int))
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants