[llvm] r175063 - [ms-inline-asm] Use an array_pod_sort, rather than a std:sort.

Chad Rosier mcrosier at apple.com
Fri Feb 15 14:58:09 PST 2013


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