Work around for Visual Studio 2013 compiler crash

[ADodds]

Summary

Hi all,

A line of code in ProcessStructReader.h:67 was causing the Visual Studio 2013 compiler to crash when compiling lldb.
As a workaround, I have a patch which breaks up this line and fixes the crash.

Could someone review this patch please, and if it looks ok check it in for me since I don't have commit access.

Thanks,
Aidan

[zturner]

I compile with vs2013 every day and have never seen this crash. Any idea
why it was crashing or how i might repro it?

[ADodds]

Hi Zachary,

Here was the error message generated from the crash:

1>ClCompile:
1> CXXFormatterFunctions.cpp
1>d:\tiplldb\llvm\tools\lldb\include\lldb\utility\processstructreader.h(75):
fatal error C1001: An internal error has occurred in the compiler.
1> (compiler file 'f:\dd\vctools\compiler\utc\src\p2\main.c', line 227)
1> To work around this problem, try simplifying or changing the
program near the locations listed above.
1> Please choose the Technical Support command on the Visual C++
1> Help menu, or open the Technical Support help file for more information
1>
1>Build FAILED.

Note the above line number is a little off due to my patch, but I
tracked it down to the following line:

m_fields[ConstString(name.c_str())] =
FieldImpl{field_type,static_cast<size_t>(bit_offset/8),static_cast<size_t>(size)};

Here is the banner from my version of cl:

1>ClCompile:
1> Microsoft (R) C/C++ Optimizing Compiler Version 18.00.21005.1 for x64
1> Copyright (C) Microsoft Corporation. All rights reserved.

My visual studio solution was generated via CMake 2.8.11.2
My visual studio version is: 12.0.21005.1 REL

I may be worth mentioning that I was directed towards this error by
others that were seeing it also, so its not isolated just to myself, as
is the 32bit/64bit stuff from my other patch.
Are you building for an x64 target or x86? I am only targeting x64.

I am also seeing this error in both Debug and Release builds.
I have used the default CMake options when generating the solution files.

Let me know if I can provide any more information.

Thanks,
Aidan

[ADodds]

I should have added that I cloned from the git mirror at the following
revisions:

llvm: 77b6849e61b8f84e1408d1c7937598cf23190d14
clang: 1a820646d22f96ebd592950169b7db170c4b2861
lldb: 0b1b86bb3c99892032b1707b2acb2221ef6b41dc

[zturner]

It must be because you're building 64-bit. I've never attempted to make
that work.

[zturner]

Comments inline.

[rfoos]

I'm having these problems on MSVC 13 update 3 with a 64 bit build.

I have a zorg builder about ready to go public, and this would help that
effort as well.

-rick

[clayborg]

Looks fine.

[ADodds]

I have revised the patch after review by Zachary.

It seems the crash was triggered specificaly when indexing m_fields via the ConstString constructed inline, so I still had to pull that out to a local variable however.

[clayborg]

Looks good.

[zturner]

Looks good, I will commit for you.

include/lldb/Utility/ProcessStructReader.h

66–67 ↗

(On Diff #17384)

Remove the #if and the comment. The code stands on its own, so I don't think this adds much value.

68–71 ↗

(On Diff #17384)

Local variable naming conventions are all lowercase, underscores between words. But no underscore at the end of the word. And we prefer to have meaningful variable names instead of letters like a, b, and c. So let's re-write this (including a few lines prior to where this patch starts) as:

size_t size = static_cast<size_t>(field_type.GetByteSize());
// no support for things larger than a uint64_t (yet)
if (size > 8)
    return;
size_t byte_index = static_cast<size_t>(bit_offset/8);
m_fields[ConstString(name.c_str()))] = FieldImpl {field_type, byte_index, size};

Note that the reason this ICE'd in the first place is because MSVC doesn't have good support for brace initialization. So even if the fix here solves the ICE, MSVC might have code generation errors. So perhaps an even better fix is to write it like this:

size_t size = static_cast<size_t>(field_type.GetByteSize());
// no support for things larger than a uint64_t (yet)
if (size > 8)
    return;
FieldImpl &field = m_fields[ConstString(name.c_str()))];
field.type = field_type;
field.offset = static_cast<size_t>(bit_offset/8);
field.size = size;

[zturner]

Ignore the comments that showed up on the last response. I was writing that before I saw your comment about the ConstString being the issue, and Phabricator has a bug where it can't delete a draft, even after refreshing the page etc.

[ADodds]

Thanks very much for committing that Zachary.

lldb/trunk/include/lldb/Utility/ProcessStructReader.h

public:
                 ClangASTType field_type = struct_type.GetFieldAtIndex(idx,name,&bit_offset,&bitfield_bit_size,&is_bitfield);
                 // no support for bitfields in here (yet)
                 if (is_bitfield)
                     return;
                 auto size = field_type.GetByteSize();
                 // no support for things larger than a uint64_t (yet)
                 if (size > 8)
                     return;
-                m_fields[ConstString(name.c_str())] = FieldImpl{field_type,static_cast<size_t>(bit_offset/8),static_cast<size_t>(size)};
+                ConstString const_name = ConstString(name.c_str());
+                size_t byte_index = static_cast<size_t>(bit_offset / 8);
+                m_fields[const_name] = FieldImpl{field_type, byte_index, size};
             }
             size_t total_size = struct_type.GetByteSize();
             lldb::DataBufferSP buffer_sp(new DataBufferHeap(total_size,0));
             Error error;
             process->ReadMemoryFromInferior(base_addr,
                                             buffer_sp->GetBytes(),
                                             total_size,
                                             error);