<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Dec 26, 2013, at 5:03 AM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="rtl"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div dir="ltr">I tested this patch with gcc 4.8.2 32 bit, Visual C++ 2013 32 bit, Visual C++ 2013 64 bits.</div><div dir="ltr"><br></div><div dir="ltr">Without the patch (current svn), gcc passes, VC/32 fails to compile, VC/64 compiles but fails the bit tests. With the patch, all compile and pass the tests.</div>
<div dir="ltr"><br></div><div dir="ltr">So the patch LGTM. Can it be committed ? </div></div></blockquote><div><br></div>I do not currently have a setup that will let me test this.</div><div>By eye, it looks fine to me.</div><div><br></div><div><blockquote type="cite"><div dir="rtl"><div dir="ltr">bittests.cpp tests several clang/gcc compiler intrinsics missing from Visual C++ which are required by libcxx. Can we put it into libcxx tests ?</div></div></blockquote><div><br></div>The test program needs a bit of work to fit into the libc++ tests.</div><div>it should fail (aka call abort()) if any of the tests fail.</div><div>An easy way to do that is to have check() call assert()</div><div>Also, it needs to pass if it is build on a non-windows machine. (not fail to compile, etc).</div><div><br></div><div>— Marshall</div><div><br></div><div><br><blockquote type="cite"><div dir="rtl">
<div dir="ltr"><br></div><div dir="ltr">Yaron</div><div dir="ltr"><br></div></div><div class="gmail_extra"><div dir="ltr"><br><br><div class="gmail_quote">2013/12/23 G M <span dir="ltr"><<a href="mailto:gmisocpp@gmail.com" target="_blank">gmisocpp@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 .8ex;border-left:1px #ccc solid;border-right:1px #ccc solid;padding-left:1ex;padding-right:1ex">Here's a revised version of my recent patch that implements yaron's suggestion to remove DWORD use. I've also made it more explicitly Win32/msvc specific which I think it practically was anyway and being explicit may be easier to manage.</blockquote>
</div><div class="HOEnZb"><div class="h5">
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Dec 24, 2013 at 8:45 AM, G M <span dir="ltr"><<a href="mailto:gmisocpp@gmail.com" target="_blank">gmisocpp@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Hi Yaron<br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Tue, Dec 24, 2013 at 2:07 AM, Yaron Keren <span dir="ltr"><<a href="mailto:yaron.keren@gmail.com" target="_blank">yaron.keren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="rtl"><div dir="ltr">Hi G M,</div><div dir="ltr">
<br></div><div dir="ltr">The DWORDs should not have been there at all as _BitScanReverse64 and _BitScanForward formally take unsigned long* and not DWORD* (although they are the same thing). <br>
</div><div dir="ltr"><br></div><div dir="ltr"><a href="http://msdn.microsoft.com/en-us/library/wfd9z0bb(v=vs.90).aspx" target="_blank">http://msdn.microsoft.com/en-us/library/wfd9z0bb(v=vs.90).aspx</a><br></div><div dir="ltr">
<br></div>
<div dir="ltr">
Use unsigned long instead of DWORD and wtypes.h will not be needed.<span><font color="#888888"><br></font></span></div><span><font color="#888888"><div dir="ltr"><br></div></font></span></div>
</blockquote></div><div>Yes I find this confusing because I'm pretty sure other documentation (which I now conveniently can't find) had the types using DWORD etc. and I, and I'm sure the original author of these functions that I'm fixing, based their code after that and not the documentation you are linking too. That's how I think the DWORD stuff got in to begin with.</div>
<div><br></div><div>In an earlier version of this patch that I posted, but seemed to get lost I removed the DWORD types etc. due to the documentation you linked to.</div><div><br></div><div>What caused me to put it back was that some headers like Winnt.h still refer to DWORD etc. But now looking at intrin.h which I can see is the one to follow doesn't use DWORD anymore if it ever did.</div>
<div><br></div><div>So I can see you are right, I'll re-submit another patch shortly that again removes DWORD.</div><div><br></div><div>Thanks.</div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
<div dir="rtl"><span><font color="#888888"><div dir="ltr"></div></font></span><div><div dir="ltr"><br></div><div class="gmail_extra"><div dir="ltr"><br><br><div class="gmail_quote">
2013/12/23 G M <span dir="ltr"><<a href="mailto:gmisocpp@gmail.com" target="_blank">gmisocpp@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><p>Hello Win32 libcxx watchers.</p><div>support.h implements the functions clz, clzl, clzll, ctz, ctzl, and ctzll functions (I hate those names) for Win32 when the compiler does not support them. So ms's cl.exe uses the implementations in support.h but clang and gcc compilers provide these implementations themselves, not support.h.</div>
<div><br></div><div>Currently these support.h implementations have a few problems.</div><div><br>With the removal of windows.h from support.h they no longer compile. Even when they did compile, they were incorrect at runtime. In more detail:</div>
<div><br></div><div>1. support.h now fails to compile for msvc (cl.exe) because it depended on things like DWORD from windows.h which was removed in a prior commit and no lighter weight replacement for windows.h was provided. This patch supplies that with wtypes.h</div>
<div><br></div><div>2. These functions aren't implemented for 32 bit targets of cl.exe but they should be as they are available in the 32 bit and 64 bit gcc and clang targets. This is because the current implementations use BitScanForward64 etc. which aren't available to 32 bit targets. This patch provides 32 bit implementations of the clt/l/ll and clz/l/ll functions to fix that</div>
<div>.</div><div>3. The above named functions current implementation in support.h is incorrect. The return values of the support.h versions don't match the gcc or clang return values when given the same inputs.</div>
<div>
<br></div><div>The attached support.h.diff patch fixes these problems.</div><p>Anyone wanting to verify the above statements: compile the attached test case bittests.cpp with MS's 32 bit compiler cl.exe against the SVN version of libcxx and it's support.h (i.e. before applying the support.h.diff patch), you will see:<br>
cl bittest.cpp -I/libcxx/include<br>c:\libcxx\include\support/win32/support.h(85) : error C2065: 'DWORD' : undeclared identifier<br><etc...> This is demonstrates problem 1 above.</p><p>If you edit that svn version of support.h and add wtypes.h and compile again, you will see the first errors vanish. but then you then you will see these errors:<br>
c:\libcxx\include\support\win32\support.h(97) : error C3861: '_BitScanReverse64': identifier not found<br>c:\libcxx\include\support\win32\support.h(112) : error C3861: '_BitScanForward64': identifier not found<br>
This demonstrates problem 2 above.</p><p>Switching to the 64 bit cl.exe compiler, if you compile with that, you should be able to run it but get these runtime errors:<br>Test ctz failed. line 23, expected value: 32, actual value: 0<br>
Test ctzl failed. line 33, expected value: 32, actual value: 0<br>Test ctzll failed. line 44, expected value: 64, actual value: 0<br>Test clz failed. line 57, expected value: 32, actual value: 0<br><snip><br>134 tests failed<br>
This demonstrates problem number 3.</p><p>If you try compiling and runing this file with g++ or clang, you should see no errors. This shows gcc and clang's implementations of the clt/clz functions are ok yet support.h's version are not.</p>
<div>If you delete support.h and re get (assuming you modified it to add wtypes.h) and then apply the support.h.diff patch and then re-compile with g++, clang++ or cl.exe in 32 bit or 64 bit mode, the test case should then run with no errors or output for all of those compilers, hopefully indicating success with no regressions and proving that all the issues stated were real and that this patch has fixed all the stated problems.</div>
<div><br></div><div>I am going on holiday the day after tomorrow and won't have access to email unfortunately so I won't be able to reply to any questions after tomorrow for about a week, but I thought I'd make this patch available in the mean time for review and anyone who wants to play with it. I'll try to reply to any early responses that come in to this patch before I go though.</div>
<div><br></div><div>Thanks<br></div></blockquote></div></div></div></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></div></div>
</blockquote></div><br></body></html>