This is an archive of the discontinued LLVM Phabricator instance.

Correct debug info bit offset calculation for big-endian targets
ClosedPublic

Authored by frej on May 2 2017, 6:08 AM.

Details

Summary

The change "[CodeView] Implement support for bit fields in
Clang" (r274201, https://reviews.llvm.org/rL274201) broke the
calculation of bit offsets for the debug info describing bitfields on
big-endian targets.

Prior to commit r274201 the debug info for bitfields got their offsets
from the ASTRecordLayout in CGDebugInfo::CollectRecordFields(), the
current field offset was then passed on to
CGDebugInfo::CollectRecordNormalField() and used directly in the
DIDerivedType.

Since commit r274201, the bit offset ending up in the DIDerivedType no
longer comes directly from the ASTRecordLayout. Instead
CGDebugInfo::CollectRecordNormalField() calls the new method
CGDebugInfo::createBitFieldType(), which in turn calls
CodeGenTypes::getCGRecordLayout().getBitFieldInfo() to fetch a
CGBitFieldInfo describing the field. The 'Offset' member of
CGBitFieldInfo is then used to calculate the bit offset of the
DIDerivedType. Unfortunately the previous and current method of
calculating the bit offset are only equivalent for little endian
targets, as CGRecordLowering::setBitFieldInfo() reverses the bit
offsets for big endian targets as the last thing it does.

A simple reproducer for this error is the following module:

struct fields {

unsigned a : 4;
unsigned b : 4;

} flags = {0x0f, 0x1};

Compiled for Mips, with commit r274200 both the DIDerivedType bit
offsets on the IR-level and the DWARF information on the ELF-level
will have the expected values: the offsets of 'a' and 'b' are 0 and 4
respectively. With r274201 the offsets are switched to 4 and 0. By
noting that the static initialization of 'flags' in both cases is the
same, we can eliminate a change in record layout as the cause of the
change in the debug info. Also compiling this example with gcc,
produces the same record layout and debug info as commit r274200.

In order to restore the previous function we extend
CGDebugInfo::createBitFieldType() to compensate for the reversal done
in CGRecordLowering::setBitFieldInfo().

Diff Detail

Repository
rL LLVM

Event Timeline

frej created this revision.May 2 2017, 6:08 AM
aprantl accepted this revision.May 2 2017, 9:10 AM

Doesn't the test need some kind of REQUIRES: mips line?
Otherwise looks fine.

This revision is now accepted and ready to land.May 2 2017, 9:10 AM
frej updated this revision to Diff 97544.May 2 2017, 10:37 PM

Added missing "REQUIRES: mips-registered-target".

rnk edited edge metadata.May 2 2017, 11:29 PM

The REQUIRES tag is unnecessary. Clang can emit IR for any target without a registered backed.

frej added a comment.May 2 2017, 11:38 PM
In D32745#744280, @rnk wrote:

The REQUIRES tag is unnecessary. Clang can emit IR for any target without a registered backed.

So should I drop it? It doesn't hurt does it?

rnk added a comment.May 3 2017, 7:59 AM
In D32745#744292, @frej wrote:
In D32745#744280, @rnk wrote:

The REQUIRES tag is unnecessary. Clang can emit IR for any target without a registered backed.

So should I drop it? It doesn't hurt does it?

Some developers configure their build to only build the x86 backend to speed it up. This test won't run in that configuration, so they won't know if it fails.

Ah. Thanks for clarifying! We should remove it, then.

rnk accepted this revision.May 3 2017, 10:21 AM

Looks good without the REQUIRES line, thanks for the fix!

It's unfortunate that this logic is here and in CodeGen/AsmPrinter/DwarfUnit.cpp, but such is life.

frej updated this revision to Diff 97773.May 3 2017, 10:52 PM

Removed 'REQUIRES'-line.

frej added a comment.Jun 7 2017, 1:35 AM

Ping, I don't have have commit permissions, so I am at the mercy of others for getting this fix committed...

This revision was automatically updated to reflect the committed changes.