Page MenuHomePhabricator

Work around for Visual Studio 2013 compiler crash
ClosedPublic

Authored by ADodds on Dec 17 2014, 3:51 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

ADodds updated this revision to Diff 17384.Dec 17 2014, 3:51 AM
ADodds retitled this revision from to Work around for Visual Studio 2013 compiler crash.
ADodds updated this object.
ADodds edited the test plan for this revision. (Show Details)
ADodds added reviewers: emaste, zturner, clayborg, domipheus.
ADodds set the repository for this revision to rL LLVM.
ADodds added a subscriber: Unknown Object (MLST).
zturner edited edge metadata.Dec 17 2014, 7:15 AM

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

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

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

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

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

Comments inline.

rfoos added a subscriber: rfoos.Dec 17 2014, 9:31 AM

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

ted added a subscriber: ted.Dec 17 2014, 9:45 AM
clayborg accepted this revision.Dec 17 2014, 9:49 AM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Dec 17 2014, 9:49 AM
ADodds updated this revision to Diff 17399.Dec 17 2014, 9:51 AM
ADodds edited edge metadata.
ADodds removed rL LLVM as the repository for this revision.

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.

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;

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.

This revision was automatically updated to reflect the committed changes.

Thanks very much for committing that Zachary.