llvm dev - Jul 2013 - [LLVMdev] [cfe-dev] llvm (hence Clang) not compiling withVisual Studio 2008

Hello,

I have just updated my svn copy of the llvm/clang repositories after quite
a long time of inactivity, and found it not compiling on Windows with
Visual Studio 2008.

The incriminated file is:

  llvm/lib/MC/MCModule.cpp

Where several calls to "std::lower_bound" are made, like:

  atom_iterator I = std::lower_bound(atom_begin(), atom_end(),
                                     Begin, AtomComp);

With:
  - "atom_iterator" being a typedef on
"std::vector<llvm::Atom*>::iterator"
  - "atom_begin()" and "atom_end" returning an
"atom_iterator"
  - "Begin" being an "uint64_t"
  - "AtomComp" being a predicate of type "bool (const
llvm::MCAtom
*,uint64_t)"

This seems to be due to an invalid implementation of the STL as provided
with Visual Studio 2008.

Indeed, the predicate given to "lower_bound" must respect the
following
rules:

  - obviously, it shall return "bool" (here: of course)

  - its first argument shall be of a type into which the type of the
dereferenced iterators can be implicitly converted (here:
"atom_iterator::operator*" returns a "llvm::Atom*", and the
first argument
of "AtomComp" is also "llvm::Atom*"

  - its second argument shall be of a type into which the type of the value
can be implicitly converted (here: "Begin" is an
"uint_64_t", as well as
the second argument of "AtomComp")

But the implementation of "std::lower_bound" in Visual Stuio 2008
relies on
a checker provided by the file "xutility", which reads:

template<class _Pr, class _Ty1, class _Ty2> inline
bool __CLRCALL_OR_CDECL _Debug_lt_pred(_Pr _Pred,
                                       _Ty1& _Left,
                                       const _Ty2& _Right,
                                       const wchar_t *_Where,
                                       unsigned int _Line)
{ // test if _Pred(_Left, _Right) and _Pred is strict weak ordering

  if (!_Pred(_Left, _Right))
    return (false);
  else if (_Pred(_Right, _Left))
    _DEBUG_ERROR2("invalid operator<", _Where, _Line);

  return (true);
}

Hence, it expects the predicate (here "_Pred") to accept as arguments
both
(_Ty1, _Ty2) and (_Ty2, _Ty1), which does not seem consistent with the
specifications mentioned above.

Solutions here:

1. consider that the implementation if effectively wrong, and modify the
documentation at http://llvm.org/docs/GettingStartedVS.html, requiring
Visual Studio 2010, i.e. replacing:

   "You will need Visual Studio 2008 or higher."

   by:

   "You will need Visual Studio 2010 or higher."

   Same comments on the respect of the standard apply

2. investigate whether there exists a way to disable the aforementioned
check;

3. modify the code in MCModule.cpp to cope with the implementation of
"lower_bound" in VS 2008.

Personally I just went for (1), i.e. switching to Visual Studio 2010, as it
was the most straightforward.

Doing so, I also had to add "#include <string>" to the file
"lib/CodeGen/CGBlocks.cpp" so that llvm/clang can compile with said
compiler, because of some obscure external template usage.

Regards,
-- 
Benoit PERROT
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130704/b6e3eae5/attachment.html>

On Thu, Jul 4, 2013 at 12:48 AM, Benoit Perrot
<benoit.noe.perrot at gmail.com>wrote:
> Hello,
>
Hi Benoit,

> I have just updated my svn copy of the llvm/clang repositories after quite
> a long time of inactivity, and found it not compiling on Windows with
> Visual Studio 2008.
>
> The incriminated file is:
>
>   llvm/lib/MC/MCModule.cpp
>
> Where several calls to "std::lower_bound" are made, like:
>
>   atom_iterator I = std::lower_bound(atom_begin(), atom_end(),
>                                      Begin, AtomComp);
>
> With:
>   - "atom_iterator" being a typedef on
"std::vector<llvm::Atom*>::iterator"
>   - "atom_begin()" and "atom_end" returning an
"atom_iterator"
>   - "Begin" being an "uint64_t"
>   - "AtomComp" being a predicate of type "bool (const
llvm::MCAtom
> *,uint64_t)"
>
> This seems to be due to an invalid implementation of the STL as provided
> with Visual Studio 2008.
>
> Indeed, the predicate given to "lower_bound" must respect the
following
> rules:
>
>   - obviously, it shall return "bool" (here: of course)
>
>    - its first argument shall be of a type into which the type of the
> dereferenced iterators can be implicitly converted (here:
> "atom_iterator::operator*" returns a "llvm::Atom*", and
the first argument
> of "AtomComp" is also "llvm::Atom*"
>
>   - its second argument shall be of a type into which the type of the
> value can be implicitly converted (here: "Begin" is an
"uint_64_t", as well
> as the second argument of "AtomComp")
>
> But the implementation of "std::lower_bound" in Visual Stuio 2008
relies
> on a checker provided by the file "xutility", which reads:
>
> template<class _Pr, class _Ty1, class _Ty2> inline
> bool __CLRCALL_OR_CDECL _Debug_lt_pred(_Pr _Pred,
>                                        _Ty1& _Left,
>                                        const _Ty2& _Right,
>                                        const wchar_t *_Where,
>                                        unsigned int _Line)
> { // test if _Pred(_Left, _Right) and _Pred is strict weak ordering
>
>   if (!_Pred(_Left, _Right))
>     return (false);
>   else if (_Pred(_Right, _Left))
>     _DEBUG_ERROR2("invalid operator<", _Where, _Line);
>
>   return (true);
> }
>
>  Hence, it expects the predicate (here "_Pred") to accept as
arguments
> both (_Ty1, _Ty2) and (_Ty2, _Ty1), which does not seem consistent with the
> specifications mentioned above.
>
> Solutions here:
>
> 1. consider that the implementation if effectively wrong, and modify the
> documentation at http://llvm.org/docs/GettingStartedVS.html, requiring
> Visual Studio 2010, i.e. replacing:
>
>    "You will need Visual Studio 2008 or higher."
>
>    by:
>
>    "You will need Visual Studio 2010 or higher."
>
>    Same comments on the respect of the standard apply
>
> 2. investigate whether there exists a way to disable the aforementioned
> check;
>
> 3. modify the code in MCModule.cpp to cope with the implementation of
> "lower_bound" in VS 2008.
>
> Personally I just went for (1), i.e. switching to Visual Studio 2010, as
> it was the most straightforward.
>
(3) Fixed in r185676.

Requiring VS 2010 for a minor problem like this (even though there are more
like it) isn’t warranted I think.

Doing so, I also had to add "#include <string>" to the
file
> "lib/CodeGen/CGBlocks.cpp" so that llvm/clang can compile with
said
> compiler, because of some obscure external template usage.
>
<string> is already included, at least by StringRef.h, so I’m curious:
what
is this obscure thing that needs including it again?

Thanks,

-- Ahmed Bougacha

> Regards,
> --
> Benoit PERROT
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130704/cb0a7c4e/attachment.html>

Hello Ahmed,

On Fri, Jul 5, 2013 at 1:43 AM, Ahmed Bougacha <ahmed.bougacha at
gmail.com>wrote:
> On Thu, Jul 4, 2013 at 12:48 AM, Benoit Perrot <
> benoit.noe.perrot at gmail.com> wrote:
>
>>
>>
> I have just updated my svn copy of the llvm/clang repositories after quite
>> a long time of inactivity, and found it not compiling on Windows with
>> Visual Studio 2008.
>> [...]
>> Solutions here:
>>
>> 1. consider that the implementation if effectively wrong, and modify
the
>> documentation at http://llvm.org/docs/GettingStartedVS.html, requiring
>> Visual Studio 2010, i.e. replacing:
>> 2. investigate whether there exists a way to disable the aforementioned
>> check;
>> 3. modify the code in MCModule.cpp to cope with the implementation of
>> "lower_bound" in VS 2008.
>>
>> Personally I just went for (1), i.e. switching to Visual Studio 2010,
as
>> it was the most straightforward.
>>
>
> (3) Fixed in r185676.
>
> Requiring VS 2010 for a minor problem like this (even though there are
> more like it) isn’t warranted I think.
>
Great!

> Doing so, I also had to add "#include <string>" to the file
>> "lib/CodeGen/CGBlocks.cpp" so that llvm/clang can compile
with said
>> compiler, because of some obscure external template usage.
>>
>
> <string> is already included, at least by StringRef.h, so I’m
curious:
> what is this obscure thing that needs including it again?
>
>
My build was in an intermediate invalid state.
Reverting said modification, cleaning completely then building again just
worked fine. Sorry for the false alarm.


Thanks!
-- 
Benoit PERROT
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130705/9d815fb3/attachment.html>

There is some historical precedence for fixing the problem with VS 
lower_bound by changing the LLVM source - when I first got LLVM to compile 
with Visual Studio, patches for unsymmetric operator < were accepted into 
the LLVM repo, and I believe it's been done several times after that as 
well.

m.
>From: Ahmed Bougacha
>Sent: Friday, July 05, 2013 1:43 AM
>To: Benoit Perrot
>Cc: cfe-dev at cs.uiuc.edu ; llvmdev at cs.uiuc.edu
>Subject: Re: [LLVMdev] [cfe-dev] llvm (hence Clang) not compiling 
>withVisual Studio 2008
>On Thu, Jul 4, 2013 at 12:48 AM, Benoit Perrot 
><benoit.noe.perrot at gmail.com> wrote:
>>3. modify the code in MCModule.cpp to cope with the implementation of 
>>"lower_bound" in VS 2008.
>>
>(3) Fixed in r185676.
>Requiring VS 2010 for a minor problem like this (even though there are more 
>like it) isn’t warranted I think.

> There is some historical precedence for fixing the problem with VS
> lower_bound by changing the LLVM source - when I first got LLVM to compile
> with Visual Studio, patches for unsymmetric operator < were accepted
into
> the LLVM repo, and I believe it's been done several times after that as
> well.
In the C++11 discussion back in January
(http://llvm.1065342.n5.nabble.com/Using-C-11-language-features-in-LLVM-itself-td53319.html)
there seemed to be some kind of consensus for 2010 being a reasonable
minimum. Perhaps this is a good time to break compatibility
officially.

Actually, whatever did happen to using C++11? No-one mentioned
anything about it after that thread.

Tim.