MSVC compiling issue

Project Infrastructure LLVM Dev List Archives
1

Hi @llvm,

building a debug version under MSVC 9 leads to a compiler error due to a mix of different types in a call to upper_bound. I have attached a hot-fix but I'm rather unsure if it should be applied as it is, since IMHO the reason is a MSVC library bug ("IMHO", because I don't know the requirements imposed to the predicate by the standard).

Best regards
Olaf Krzikalla

Index: lib/CodeGen/LiveInterval.cpp

2

Olaf Krzikalla <Olaf.Krzikalla@tu-dresden.de> writes:

Hi @llvm,

building a debug version under MSVC 9 leads to a compiler error due to a
mix of different types in a call to upper_bound. I have attached a
hot-fix but I'm rather unsure if it should be applied as it is, since
IMHO the reason is a MSVC library bug ("IMHO", because I don't know the
requirements imposed to the predicate by the standard).

I think that the original author just missed a `const'. Fixed in r127245.

3

Hi @llvm,

building a debug version under MSVC 9 leads to a compiler error due to a
mix of different types in a call to upper_bound. I have attached a
hot-fix but I'm rather unsure if it should be applied as it is, since
IMHO the reason is a MSVC library bug ("IMHO", because I don't know the
requirements imposed to the predicate by the standard).

I hoped the symmetric methods would be enough to trick MSVC into compiling it.

Is that extra method getting called? What happens if you stick assert(0) in there?

4

Thanks, applied as r127264.

Does it pass the unit tests with the patch?

/jakob

5

Hi @llvm,

Is that extra method getting called? What happens if you stick assert(0) in there?

That won't work either (that is, the assert fires). In debug mode the MSVC lib tries to test the ordering of the sequence. And it uses the yielded predicate for this (which in this particular case is a very bad idea).

I hoped the symmetric methods would be enough to trick MSVC into compiling it.

Does that mean, that gcc actually only needs

bool operator()(const LiveRange&A, SlotIndex B) ?

According to C++(2003) 25.0.0.8 the answer is "yes", however that section talks about BinaryPredicate and not Compare. The standard is rather unclear at this point and I'm going over to comp.std.c++ to ask.

Best regards
Olaf Krzikalla

6

Hi @llvm,

Is that extra method getting called? What happens if you stick assert(0) in there?

That won't work either (that is, the assert fires). In debug mode the MSVC lib tries to test the ordering of the sequence. And it uses the yielded predicate for this (which in this particular case is a very bad idea).

I see. I guess that makes sense if it is written assuming symmetric types.

I hoped the symmetric methods would be enough to trick MSVC into compiling it.

Does that mean, that gcc actually only needs

bool operator()(const LiveRange&A, SlotIndex B) ?

Actually, the other way around.

According to C++(2003) 25.0.0.8 the answer is "yes", however that section talks about BinaryPredicate and not Compare. The standard is rather unclear at this point and I'm going over to comp.std.c++ to ask.

Howard Hinnant was kind enough to clarify this a while back.

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-August/010379.html

7

MSVC 9 asserts that the comparision function is symmetric, because of
what was unclear wording in the standard (there are posts explaining
this in the boost archive for those interested). The workaround is to
supply the other overload of the comparator.

Sorry Jacob, didn't reply all last time.

8

That's what I did initially, but apparently the debug library also verifies that the array is ordered, so three version was required.

I finally gave up and stole Howard's upper_bound(), see r127522. It is much less code to write your own algorithms instead of using the STL. Tell your kids!

Interestingly, X86FloatingPoint.cpp has an asymmetric lower_bound that hasn't caused problems. It uses operator<(), though

/jakob