Advice on a VStudio specific patch

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Advice on a VStudio specific patch

Chuck Rose III

Hola LLVMers,

 

Our project is cross platform and on Windows we use VStudio 2005.  VStudio presents a couple of issues related around it’s STL implementation and also it’s non-respect for the no-return semantic of abort(). 

 

I’ve fixed it locally, but I’d like to send a patch so I don’t have to do this every time I update from the source repository.  So…. if I’m fixing something for a specific compiler, do you think I should just do so for all compilers or should I put the differences in a #ifdef check for VStudio?

 

Thanks,

Chuck.


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

Chris Lattner
On Thu, 31 May 2007, Chuck Rose III wrote:
> Our project is cross platform and on Windows we use VStudio 2005.
> VStudio presents a couple of issues related around it's STL
> implementation and also it's non-respect for the no-return semantic of
> abort().

Ok.  We want the source to be portable, so it's goodness to get these
fixes into the main tree.

> I've fixed it locally, but I'd like to send a patch so I don't have to
> do this every time I update from the source repository.  So.... if I'm
> fixing something for a specific compiler, do you think I should just do
> so for all compilers or should I put the differences in a #ifdef check
> for VStudio?

Can you send one example of what you're thinking of?  We prefer to keep
the main code #ifdef free, moving compiler-specific code to
include/llvm/Support/Compiler.h.  Depending on what you mean, this may or
may not make sense though :)

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

Chuck Rose III
Here are the two problem areas:

RegisterInfoEmitter.cpp

  // Emit the subregister + index mapping function based on the
information
  // calculated above.
  OS << "unsigned " << ClassName
     << "::getSubReg(unsigned RegNo, unsigned Index) const {\n"
     << "  switch (RegNo) {\n"
     << "  default: abort(); break;\n";
...
  OS << "  };\n";
  OS << "  return 0; // Visual Studio 2005 does not respect the
no-return semantics of abort\n";
  OS << "}\n\n";
 
Need this because otherwise the emitted function will fail to compile in
VStudio.

PredicateSimplifier.cpp:

The other problem is subtle.  In the Edge class (for example) this
function will fail to compile in debug.

      iterator find(unsigned n, ETNode *Subtree) {
        iterator E = end();
        for (iterator I = std::lower_bound(begin(), E, n);
             I != E && I->To == n; ++I) {
          if (Subtree->DominatedBy(I->Subtree))
            return I;
        }
        return E;
      }

The debug paths through VStudio's STL through the std::lower_bound
function get to this template:

        template<class _Ty1, class _Ty2> inline
        bool __CLRCALL_OR_CDECL _Debug_lt(const _Ty1& _Left, const _Ty2&
_Right,
                const wchar_t *_Where, unsigned int _Line)
        { // test if _Left < _Right and operator< is strict weak
ordering
        if (!(_Left < _Right))
                return (false);
        else if (_Right < _Left)
                _DEBUG_ERROR2("invalid operator<", _Where, _Line);
        return (true);
        }

The critical feature is that the A<B (A an Edge and B an unsigned)
operator also ends up doing B<A in order to check that the comparator
isn't wonky.  And since the B<A function wasn't written in
PredicateSimplifier.cpp, I needed to add some stuff like this:

        // this one's already there
      bool operator<(unsigned to) const {
        return To < to;
      }

        // these two together get me the unsigned < Edge operation
      bool operator>(unsigned to) const {
        return To > to;
      }

      friend bool operator<(unsigned to, const Edge& edge) {
        return edge.operator>(to);
      }

There is a similar set of additions for ScopedRange.

Adding the alternate comparitors and the return won't make things less
portable, but it does crudify the code somewhat in order to get around
the limitations of the compiler.  Having a bunch of #ifdefs for VStudio
probably is crudier still, however. :-)

What do you think?

Thanks,
Chuck.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]]
On Behalf Of Chris Lattner
Sent: Thursday, May 31, 2007 4:00 PM
To: LLVM Developers Mailing List
Subject: Re: [LLVMdev] Advice on a VStudio specific patch

On Thu, 31 May 2007, Chuck Rose III wrote:
> Our project is cross platform and on Windows we use VStudio 2005.
> VStudio presents a couple of issues related around it's STL
> implementation and also it's non-respect for the no-return semantic of
> abort().

Ok.  We want the source to be portable, so it's goodness to get these
fixes into the main tree.

> I've fixed it locally, but I'd like to send a patch so I don't have to
> do this every time I update from the source repository.  So.... if I'm
> fixing something for a specific compiler, do you think I should just
do
> so for all compilers or should I put the differences in a #ifdef check
> for VStudio?

Can you send one example of what you're thinking of?  We prefer to keep
the main code #ifdef free, moving compiler-specific code to
include/llvm/Support/Compiler.h.  Depending on what you mean, this may
or
may not make sense though :)

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

Chris Lattner
On Thu, 31 May 2007, Chuck Rose III wrote:

> Here are the two problem areas:
> RegisterInfoEmitter.cpp
>  // Emit the subregister + index mapping function based on the
> information
>  // calculated above.
>  OS << "unsigned " << ClassName
>     << "::getSubReg(unsigned RegNo, unsigned Index) const {\n"
>     << "  switch (RegNo) {\n"
>     << "  default: abort(); break;\n";
> ...
>  OS << "  };\n";
>  OS << "  return 0; // Visual Studio 2005 does not respect the
> no-return semantics of abort\n";
>  OS << "}\n\n";
>
> Need this because otherwise the emitted function will fail to compile in
> VStudio.

Ok.  For this, I suggest just removing the break after the abort.  visual
studio will just think it falls through into the next case, which is
harmless.

> operator also ends up doing B<A in order to check that the comparator
> isn't wonky.  And since the B<A function wasn't written in
> PredicateSimplifier.cpp, I needed to add some stuff like this:
>
> // these two together get me the unsigned < Edge operation
>      bool operator>(unsigned to) const {
>        return To > to;
>      }
> There is a similar set of additions for ScopedRange.

Okay,  those sound easy and simple to add unconditionally.

> Adding the alternate comparitors and the return won't make things less
> portable, but it does crudify the code somewhat in order to get around
> the limitations of the compiler.  Having a bunch of #ifdefs for VStudio
> probably is crudier still, however. :-)

Right, sounds good.  Please add them unconditionally, so no #ifdef's
required :)

-Chris

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> On Behalf Of Chris Lattner
> Sent: Thursday, May 31, 2007 4:00 PM
> To: LLVM Developers Mailing List
> Subject: Re: [LLVMdev] Advice on a VStudio specific patch
>
> On Thu, 31 May 2007, Chuck Rose III wrote:
>> Our project is cross platform and on Windows we use VStudio 2005.
>> VStudio presents a couple of issues related around it's STL
>> implementation and also it's non-respect for the no-return semantic of
>> abort().
>
> Ok.  We want the source to be portable, so it's goodness to get these
> fixes into the main tree.
>
>> I've fixed it locally, but I'd like to send a patch so I don't have to
>> do this every time I update from the source repository.  So.... if I'm
>> fixing something for a specific compiler, do you think I should just
> do
>> so for all compilers or should I put the differences in a #ifdef check
>> for VStudio?
>
> Can you send one example of what you're thinking of?  We prefer to keep
> the main code #ifdef free, moving compiler-specific code to
> include/llvm/Support/Compiler.h.  Depending on what you mean, this may
> or
> may not make sense though :)
>
> -Chris
>
>

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

me22
In reply to this post by Chuck Rose III
On 31/05/07, Chuck Rose III <[hidden email]> wrote:

> Here are the two problem areas:
>
> RegisterInfoEmitter.cpp
>
>   // Emit the subregister + index mapping function based on the
> information
>   // calculated above.
>   OS << "unsigned " << ClassName
>      << "::getSubReg(unsigned RegNo, unsigned Index) const {\n"
>      << "  switch (RegNo) {\n"
>      << "  default: abort(); break;\n";
> ...
>   OS << "  };\n";
>   OS << "  return 0; // Visual Studio 2005 does not respect the
> no-return semantics of abort\n";
>   OS << "}\n\n";
>
> Need this because otherwise the emitted function will fail to compile in
> VStudio.
>
This might be more subtle than it seems.  I'm assuming that since that
actually fails, you have warnings-as-errors on.  In compilers with
good flow analysis that do have noreturn abort, the extra return
should trigger an "unreachable code" warning, which will also prevent
compilation with treat warnings as errors on...

I know boost has a BOOST_UNREACHABLE_RETURN(x) macro for that
situation; Perhaps LLVM should have something similar?

~ Scott McMurray
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

Chuck Rose III
Nope, it generates an error, not a warning-as-error.  (LLVM generates a whole host of warnings when run through the VStudio STL implementation).  I misspoke earlier about VStudio ignoring noreturn.   I investigated further and found that the implementation of abort in VStudio 2k5 is not tagged noreturn, hence the error.

Thanks,
Chuck.


-----Original Message-----
From: [hidden email] on behalf of me22
Sent: Thu 5/31/2007 7:11 PM
To: LLVM Developers Mailing List
Subject: Re: [LLVMdev] Advice on a VStudio specific patch
 
On 31/05/07, Chuck Rose III <[hidden email]> wrote:

> Here are the two problem areas:
>
> RegisterInfoEmitter.cpp
>
>   // Emit the subregister + index mapping function based on the
> information
>   // calculated above.
>   OS << "unsigned " << ClassName
>      << "::getSubReg(unsigned RegNo, unsigned Index) const {\n"
>      << "  switch (RegNo) {\n"
>      << "  default: abort(); break;\n";
> ...
>   OS << "  };\n";
>   OS << "  return 0; // Visual Studio 2005 does not respect the
> no-return semantics of abort\n";
>   OS << "}\n\n";
>
> Need this because otherwise the emitted function will fail to compile in
> VStudio.
>
This might be more subtle than it seems.  I'm assuming that since that
actually fails, you have warnings-as-errors on.  In compilers with
good flow analysis that do have noreturn abort, the extra return
should trigger an "unreachable code" warning, which will also prevent
compilation with treat warnings as errors on...

I know boost has a BOOST_UNREACHABLE_RETURN(x) macro for that
situation; Perhaps LLVM should have something similar?

~ Scott McMurray
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

Chris Lattner
In reply to this post by me22
On Thu, 31 May 2007, me22 wrote:
> I know boost has a BOOST_UNREACHABLE_RETURN(x) macro for that
> situation; Perhaps LLVM should have something similar?

This sounds like overkill to me :).  LLVM has lots of other similar
examples.

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Advice on a VStudio specific patch

me22
In reply to this post by Chuck Rose III
On 01/06/07, Chuck Rose III <[hidden email]> wrote:
> Nope, it generates an error, not a warning-as-error.  (LLVM generates a whole host of warnings when run through the VStudio STL implementation).  I misspoke earlier about VStudio ignoring noreturn.   I investigated further and found that the implementation of abort in VStudio 2k5 is not tagged noreturn, hence the error.
>
That doesn't seem conforming to me...

int foo() {}
int main(int argc, char **argv) { if (argc < 0) return foo(); }

is, afaik, a perfectly well-defined C++ program, and issuing fatal
diagnostics on well-formed programs sure doesn't seem kosher.

~ Scott McMurray
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev