llvm dev - Aug 2006 - [LLVMdev] Built LLVM 1.8 on VC8, invalid iterator issue/fix, some questions

Hello,

I've managed to get LLVM 1.8 to build properly with MSVC8 with some
effort. Most of the changes necessary were very minor C++ usage issues;
for instance, VC8 can't deal with prototyping something that's really a
class as 'struct Foo;' or vice versa (it creates issues with matching
function signatures during linking.) Additionally, I had to do quite a few
updates to the included VC7.1 project file after doing an upgrade to a VC8
project (the project file appears to be somewhat out of date.)

However, there appears to be some issue with using an invalid iterator in
the X86 backend that appears when I build with VC8. I've managed to fix it
by duplicating the std::vector in question before iterating through it,
but I'm too new to the code to be able to say whether it's the
"right"
fix. The offending bit of generated code is this area from
X86GenDAGISel.inc, line 84+:

	// ReplaceHandles - Replace all the handles with the real target
	// specific nodes.
	void ReplaceHandles() {
	  for (std::map<SDOperand, SDOperand>::iterator I = ReplaceMap.begin(),
	        E = ReplaceMap.end(); I != E; ++I) {
	    SDOperand From = I->first;
	    SDOperand To   = I->second;
	    for (SDNode::use_iterator UI = From.Val->use_begin(), E
From.Val->use_end(); UI != E; ++UI) {
	      SDNode *Use = *UI;
	      std::vector<SDOperand> Ops;
	      for (unsigned i = 0, e = Use->getNumOperands(); i != e; ++i){
	        SDOperand O = Use->getOperand(i);
	        if (O.Val == From.Val)
	          Ops.push_back(To);
	        else
	          Ops.push_back(O);
	      }
	      SDOperand U = SDOperand(Use, 0);
	      CurDAG->UpdateNodeOperands(U, Ops); // *** This invalidates the UI
iterator ***
	    }
	  }
	}

This causes lli to crash on the ++UI in the for loop (assert failure in
debug mode, hard crash nearby in release mode) if I try to run bytecode
generated from this LLVM code:

	%blah = global int 5

	implementation

	int %main()
	{
		%foo = load int* %blah

		; Causes crash if I do this instead of
		; a constant like 'ret int 0'
		ret int %foo
	}

My hack around the situation was to copy From.Val into a temporary
std::vector<SDNode*>, by modifying the code that generates that code
(TableGen/DAGISelEmitter.cpp), but that's got a performance penalty:

	// ...
	std::vector<SDNode*> tempnodes(From.Val->use_begin(),
From.Val->use_end());
    for (SDNode::use_iterator UI = tempnodes.begin(); UI !tempnodes.end(); UI++)
{
    // ...

I could preallocate the temporary vector to a certain size to help a
little, but ideally there's no copying of vectors at all.

NOTE: I haven't tried running the testsuite on the fixed binaries to see
if the build is 100% working yet, but at least that issue appears
temporarily solved.

Anyway, I've got a few questions:
1) First thing, is VC8 support desired at the moment? If so I can try to
put together a patch, although I'd likely have to install VS7.1 to edit
the project file while maintaining compatibility with 7.1.
2) Could someone who knows more about the context of the invalid iterator
issue tell me what the 'right' fix would be? Is this just something
g++/libstdc++ happens to let pass silently and it really needs changing,
or is VC8 screwing it up somehow? If it does needs changing, what's the
proper way of doing it?
3) Should I be posting this to bugzilla instead? ;)

Thanks,
Scott Hilbert

srhilber at ncsu.edu wrote:
> However, there appears to be some issue with using an invalid iterator in
> the X86 backend that appears when I build with VC8. I've managed to fix
it
> by duplicating the std::vector in question before iterating through it,
> but I'm too new to the code to be able to say whether it's the
"right"
> fix. The offending bit of generated code is this area from
> X86GenDAGISel.inc, line 84+:
If you can take the debug time hit, it might be worth your while to
install STLport, recompile using the STLport headers and use its STL
debug mode. That might provide insight as to whether the issue is with
MSes version of STL (most likely) or a code usage problem (less likely).
Either way, it'll help determine the problem. If the iterator works with
STLport, then the problem is with MS STL.

On Thu, 24 Aug 2006 srhilber at ncsu.edu wrote:
> I've managed to get LLVM 1.8 to build properly with MSVC8 with some
> effort. Most of the changes necessary were very minor C++ usage issues;
> for instance, VC8 can't deal with prototyping something that's
really a
> class as 'struct Foo;' or vice versa (it creates issues with
matching
> function signatures during linking.)
Can you send in a patch for this?  It would be good to get this fixed in 
CVS.
> However, there appears to be some issue with using an invalid iterator in
> the X86 backend that appears when I build with VC8. I've managed to fix
it
> by duplicating the std::vector in question before iterating through it,
> but I'm too new to the code to be able to say whether it's the
"right"
> fix. The offending bit of generated code is this area from
> X86GenDAGISel.inc, line 84+:
Ok, see below.
> Anyway, I've got a few questions:
> 1) First thing, is VC8 support desired at the moment?
Yes!
> If so I can try to
> put together a patch, although I'd likely have to install VS7.1 to edit
> the project file while maintaining compatibility with 7.1.
Please do.  I'd much prefer that we keep 7.1 project files, just for 
compatibility with more VC versions.
> 2) Could someone who knows more about the context of the invalid iterator
> issue tell me what the 'right' fix would be? Is this just something
> g++/libstdc++ happens to let pass silently and it really needs changing,
> or is VC8 screwing it up somehow? If it does needs changing, what's the
> proper way of doing it?
The right fix, I believe, is to update to CVS.  The entire problematic 
piece of code is gone. :)

-Chris

-- 
http://nondot.org/sabre/
http://llvm.org/

> Can you send in a patch for this?  It would be good to get this fixed in
> CVS.
[...]
> Please do.  I'd much prefer that we keep 7.1 project files, just for
> compatibility with more VC versions.
I will try to put one together soon, free time permitting. I'll have to
find my VC 2003 disc and choose a victim machine first, and then go
through and find all the missing and moved files again. Other than that,
it's mostly very simple one-line changes to source files.
> The right fix, I believe, is to update to CVS.  The entire problematic
> piece of code is gone. :)
(Slamming of head against desk ensues)
Oh well, at least it gave me some STL debugging practice...

srhilber at ncsu.edu wrote:
> Hello,
> 
> I've managed to get LLVM 1.8 to build properly with MSVC8 with some
> effort. Most of the changes necessary were very minor C++ usage issues;
> for instance, VC8 can't deal with prototyping something that's
really a
> class as 'struct Foo;' or vice versa (it creates issues with
matching
> function signatures during linking.) Additionally, I had to do quite a few
> updates to the included VC7.1 project file after doing an upgrade to a VC8
> project (the project file appears to be somewhat out of date.)
I did the same thing a while ago, and submitted some patches. The problem with
structs and classes crop up every now and
again when someone adds new code, I fixed lots of these when LLVM was first
ported to VC7 (which has the same issue).
> However, there appears to be some issue with using an invalid iterator in
> the X86 backend that appears when I build with VC8. I've managed to fix
it
> by duplicating the std::vector in question before iterating through it,
> but I'm too new to the code to be able to say whether it's the
"right"
> fix.
The VC8 STL implementation is basically asserting everything when running in
debug mode - I submitted patches for an
issue where it checks that the operator < is antisymmetric ( given a < b
it would assert(! b < a) ) which doesn't work
if the operator is implemented so it can only compare your class to an int and
not the other way around. Sometimes it's
a bit too restrictive, but usually the code it complains about will benefit from
a cleanup anyway.

You were asking if a VC8 port is desired, and the answer is definitly
'yes' - patches for VC8 have already been accepted
and at least I'm using the VC8 port (... of release 1.5) I will upgrade to
the current release when I have some free
time, but the 1.5 release is working fine for us so at the moment it's one
of those "don't fix what isn't broken"
situations.

m.