[llvm] r175063 - [ms-inline-asm] Use an array_pod_sort, rather than a std:sort.
Timur Iskhodzhanov
timurrrr at google.com
Fri Feb 15 22:40:45 PST 2013
Yes, my bot is green again since r175317 - thanks!
2013/2/16 Chad Rosier <mcrosier at apple.com>:
> I believe r175317 should take care of things. Thanks for the great test case.
>
> Chad
>
> On Feb 15, 2013, at 10:37 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>
>> I can do better than that =P
>>
>> Here's a minimized repro:
>> ---------
>> int f(int a, int b) {
>> __asm{
>> mov ecx, b;
>> mov eax, dword ptr [a];
>> mov edx, dword ptr [a+4];
>> shld edx, eax, cl;
>> shl eax, cl;
>> }
>> }
>> ---------
>> clang -Xclang -cxx-abi -Xclang microsoft source.cpp
>>
>> 2013/2/15 Chad Rosier <mcrosier at apple.com>:
>>> Would you mind attaching the preprocessed source and crash script?
>>>
>>> clang: note: diagnostic msg: C:/Users/timurrrr/AppData/Local/Temp/windowsh_pass-036380.cpp
>>> clang: note: diagnostic msg: C:/Users/timurrrr/AppData/Local/Temp/windowsh_pass-036380.sh
>>>
>>> Chad
>>>
>>> On Feb 15, 2013, at 9:39 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>
>>>> Attached is the stacktrace just in case
>>>>
>>>> 2013/2/15 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>> OK, here we go.
>>>>>
>>>>> The crash happens if you "#include <windows.h>" on a machine with
>>>>> VS2010 installed (other versions might be affected too).
>>>>>
>>>>> Here's a patch that adds an assert that crashes:
>>>>> ---------------------------------------------------------------------------------
>>>>> diff --git lib/MC/MCParser/AsmParser.cpp lib/MC/MCParser/AsmParser.cpp
>>>>> index 2cce8b0..1cfd5f6 100644
>>>>> --- lib/MC/MCParser/AsmParser.cpp
>>>>> +++ lib/MC/MCParser/AsmParser.cpp
>>>>> @@ -4174,8 +4174,10 @@ bool AsmParser::ParseMSInlineAsm(void *AsmLoc,
>>>>> std::string &AsmString,
>>>>>
>>>>> // Emit everything up to the immediate/expression. If the previous rewrite
>>>>> // was a size directive, then this has already been done.
>>>>> - if (PrevKind != AOK_SizeDirective)
>>>>> + if (PrevKind != AOK_SizeDirective) {
>>>>> + assert(Loc >= Start);
>>>>> OS << StringRef(Start, Loc - Start);
>>>>> + }
>>>>> PrevKind = Kind;
>>>>>
>>>>> // Skip the original expression.
>>>>> ---------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> I hope that's enough info to debug :)
>>>>>
>>>>>
>>>>> FYI The bug was hard to debug as a substantial part of memory was
>>>>> corrupted (including EBPs, return addresses etc)
>>>>> so I propose something like this to prevent such bugs from re-appearing:
>>>>> ---------------------------------------------------------------------------------
>>>>> diff --git include/llvm/ADT/StringRef.h include/llvm/ADT/StringRef.h
>>>>> index 1e21d92..077c056 100644
>>>>> --- include/llvm/ADT/StringRef.h
>>>>> +++ include/llvm/ADT/StringRef.h
>>>>> @@ -84,6 +84,7 @@ namespace llvm {
>>>>> : Data(data), Length(length) {
>>>>> assert((data || length == 0) &&
>>>>> "StringRef cannot be built from a NULL argument with non-null length");
>>>>> + assert(length < 0x80000000 && "length looks suspicious");
>>>>> }
>>>>>
>>>>> /// Construct a string ref from an std::string.
>>>>> ---------------------------------------------------------------------------------
>>>>>
>>>>> 2013/2/15 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>>> FYI this crashes on my Windows bot.
>>>>>>
>>>>>> I'll update with the details when I reproduce locally.
>>>>>>
>>>>>> 2013/2/13 Chad Rosier <mcrosier at apple.com>:
>>>>>>> Author: mcrosier
>>>>>>> Date: Wed Feb 13 12:38:58 2013
>>>>>>> New Revision: 175063
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=175063&view=rev
>>>>>>> Log:
>>>>>>> [ms-inline-asm] Use an array_pod_sort, rather than a std:sort.
>>>>>>>
>>>>>>>
>>>>>>> Modified:
>>>>>>> llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>>>>
>>>>>>> Modified: llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCParser/AsmParser.cpp?rev=175063&r1=175062&r2=175063&view=diff
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/MC/MCParser/AsmParser.cpp (original)
>>>>>>> +++ llvm/trunk/lib/MC/MCParser/AsmParser.cpp Wed Feb 13 12:38:58 2013
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>
>>>>>>> #include "llvm/ADT/APFloat.h"
>>>>>>> #include "llvm/ADT/SmallString.h"
>>>>>>> +#include "llvm/ADT/STLExtras.h"
>>>>>>> #include "llvm/ADT/StringMap.h"
>>>>>>> #include "llvm/ADT/Twine.h"
>>>>>>> #include "llvm/MC/MCAsmInfo.h"
>>>>>>> @@ -4029,8 +4030,14 @@ bool AsmParser::ParseDirectiveMSAlign(SM
>>>>>>> return false;
>>>>>>> }
>>>>>>>
>>>>>>> -bool AsmStringSort (AsmRewrite A, AsmRewrite B) {
>>>>>>> - return A.Loc.getPointer() < B.Loc.getPointer();
>>>>>>> +static int RewritesSort (const void *A, const void *B) {
>>>>>>> + const AsmRewrite *AsmRewriteA = static_cast<const AsmRewrite*>(A);
>>>>>>> + const AsmRewrite *AsmRewriteB = static_cast<const AsmRewrite*>(B);
>>>>>>> + if (AsmRewriteA->Loc.getPointer() < AsmRewriteB->Loc.getPointer())
>>>>>>> + return -1;
>>>>>>> + if (AsmRewriteB->Loc.getPointer() < AsmRewriteA->Loc.getPointer())
>>>>>>> + return 1;
>>>>>>> + return 0;
>>>>>>> }
>>>>>>>
>>>>>>> bool AsmParser::ParseMSInlineAsm(void *AsmLoc, std::string &AsmString,
>>>>>>> @@ -4157,7 +4164,7 @@ bool AsmParser::ParseMSInlineAsm(void *A
>>>>>>> AsmRewriteKind PrevKind = AOK_Imm;
>>>>>>> raw_string_ostream OS(AsmStringIR);
>>>>>>> const char *Start = SrcMgr.getMemoryBuffer(0)->getBufferStart();
>>>>>>> - std::sort (AsmStrRewrites.begin(), AsmStrRewrites.end(), AsmStringSort);
>>>>>>> + array_pod_sort (AsmStrRewrites.begin(), AsmStrRewrites.end(), RewritesSort);
>>>>>>> for (SmallVectorImpl<struct AsmRewrite>::iterator
>>>>>>> I = AsmStrRewrites.begin(), E = AsmStrRewrites.end(); I != E; ++I) {
>>>>>>> const char *Loc = (*I).Loc.getPointer();
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>> 2013/2/15 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>> OK, here we go.
>>>>>
>>>>> The crash happens if you "#include <windows.h>" on a machine with
>>>>> VS2010 installed (other versions might be affected too).
>>>>>
>>>>> Here's a patch that adds an assert that crashes:
>>>>> ---------------------------------------------------------------------------------
>>>>> diff --git lib/MC/MCParser/AsmParser.cpp lib/MC/MCParser/AsmParser.cpp
>>>>> index 2cce8b0..1cfd5f6 100644
>>>>> --- lib/MC/MCParser/AsmParser.cpp
>>>>> +++ lib/MC/MCParser/AsmParser.cpp
>>>>> @@ -4174,8 +4174,10 @@ bool AsmParser::ParseMSInlineAsm(void *AsmLoc,
>>>>> std::string &AsmString,
>>>>>
>>>>> // Emit everything up to the immediate/expression. If the previous rewrite
>>>>> // was a size directive, then this has already been done.
>>>>> - if (PrevKind != AOK_SizeDirective)
>>>>> + if (PrevKind != AOK_SizeDirective) {
>>>>> + assert(Loc >= Start);
>>>>> OS << StringRef(Start, Loc - Start);
>>>>> + }
>>>>> PrevKind = Kind;
>>>>>
>>>>> // Skip the original expression.
>>>>> ---------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> I hope that's enough info to debug :)
>>>>>
>>>>>
>>>>> FYI The bug was hard to debug as a substantial part of memory was
>>>>> corrupted (including EBPs, return addresses etc)
>>>>> so I propose something like this to prevent such bugs from re-appearing:
>>>>> ---------------------------------------------------------------------------------
>>>>> diff --git include/llvm/ADT/StringRef.h include/llvm/ADT/StringRef.h
>>>>> index 1e21d92..077c056 100644
>>>>> --- include/llvm/ADT/StringRef.h
>>>>> +++ include/llvm/ADT/StringRef.h
>>>>> @@ -84,6 +84,7 @@ namespace llvm {
>>>>> : Data(data), Length(length) {
>>>>> assert((data || length == 0) &&
>>>>> "StringRef cannot be built from a NULL argument with non-null length");
>>>>> + assert(length < 0x80000000 && "length looks suspicious");
>>>>> }
>>>>>
>>>>> /// Construct a string ref from an std::string.
>>>>> ---------------------------------------------------------------------------------
>>>>>
>>>>> 2013/2/15 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>>> FYI this crashes on my Windows bot.
>>>>>>
>>>>>> I'll update with the details when I reproduce locally.
>>>>>>
>>>>>> 2013/2/13 Chad Rosier <mcrosier at apple.com>:
>>>>>>> Author: mcrosier
>>>>>>> Date: Wed Feb 13 12:38:58 2013
>>>>>>> New Revision: 175063
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=175063&view=rev
>>>>>>> Log:
>>>>>>> [ms-inline-asm] Use an array_pod_sort, rather than a std:sort.
>>>>>>>
>>>>>>>
>>>>>>> Modified:
>>>>>>> llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>>>>
>>>>>>> Modified: llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCParser/AsmParser.cpp?rev=175063&r1=175062&r2=175063&view=diff
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/MC/MCParser/AsmParser.cpp (original)
>>>>>>> +++ llvm/trunk/lib/MC/MCParser/AsmParser.cpp Wed Feb 13 12:38:58 2013
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>
>>>>>>> #include "llvm/ADT/APFloat.h"
>>>>>>> #include "llvm/ADT/SmallString.h"
>>>>>>> +#include "llvm/ADT/STLExtras.h"
>>>>>>> #include "llvm/ADT/StringMap.h"
>>>>>>> #include "llvm/ADT/Twine.h"
>>>>>>> #include "llvm/MC/MCAsmInfo.h"
>>>>>>> @@ -4029,8 +4030,14 @@ bool AsmParser::ParseDirectiveMSAlign(SM
>>>>>>> return false;
>>>>>>> }
>>>>>>>
>>>>>>> -bool AsmStringSort (AsmRewrite A, AsmRewrite B) {
>>>>>>> - return A.Loc.getPointer() < B.Loc.getPointer();
>>>>>>> +static int RewritesSort (const void *A, const void *B) {
>>>>>>> + const AsmRewrite *AsmRewriteA = static_cast<const AsmRewrite*>(A);
>>>>>>> + const AsmRewrite *AsmRewriteB = static_cast<const AsmRewrite*>(B);
>>>>>>> + if (AsmRewriteA->Loc.getPointer() < AsmRewriteB->Loc.getPointer())
>>>>>>> + return -1;
>>>>>>> + if (AsmRewriteB->Loc.getPointer() < AsmRewriteA->Loc.getPointer())
>>>>>>> + return 1;
>>>>>>> + return 0;
>>>>>>> }
>>>>>>>
>>>>>>> bool AsmParser::ParseMSInlineAsm(void *AsmLoc, std::string &AsmString,
>>>>>>> @@ -4157,7 +4164,7 @@ bool AsmParser::ParseMSInlineAsm(void *A
>>>>>>> AsmRewriteKind PrevKind = AOK_Imm;
>>>>>>> raw_string_ostream OS(AsmStringIR);
>>>>>>> const char *Start = SrcMgr.getMemoryBuffer(0)->getBufferStart();
>>>>>>> - std::sort (AsmStrRewrites.begin(), AsmStrRewrites.end(), AsmStringSort);
>>>>>>> + array_pod_sort (AsmStrRewrites.begin(), AsmStrRewrites.end(), RewritesSort);
>>>>>>> for (SmallVectorImpl<struct AsmRewrite>::iterator
>>>>>>> I = AsmStrRewrites.begin(), E = AsmStrRewrites.end(); I != E; ++I) {
>>>>>>> const char *Loc = (*I).Loc.getPointer();
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> <crashstack.txt>
>>>
>
More information about the llvm-commits
mailing list