Re: [Lldb-commits] [PATCH] D18982: Handle bit fields on big-endian systems correctly
This revision was automatically updated to reflect the committed changes. Closed by commit rL266312: Handle bit fields on big-endian systems correctly (authored by uweigand). Changed prior to commit: http://reviews.llvm.org/D18982?vs=53605&id=53711#toc Repository: rL LLVM http://reviews.llvm.org/D18982 Files: lldb/trunk/include/lldb/Core/DataExtractor.h lldb/trunk/source/Core/DataExtractor.cpp lldb/trunk/source/Core/ValueObject.cpp lldb/trunk/unittests/Core/CMakeLists.txt lldb/trunk/unittests/Core/DataExtractorTest.cpp Index: lldb/trunk/include/lldb/Core/DataExtractor.h === --- lldb/trunk/include/lldb/Core/DataExtractor.h +++ lldb/trunk/include/lldb/Core/DataExtractor.h @@ -763,8 +763,10 @@ /// /// @param[in] bitfield_bit_offset /// The bit offset of the bitfield value in the extracted -/// integer (the number of bits to shift the integer to the -/// right). +/// integer. For little-endian data, this is the offset of +/// the LSB of the bitfield from the LSB of the integer. +/// For big-endian data, this is the offset of the MSB of the +/// bitfield from the MSB of the integer. /// /// @return /// The unsigned bitfield integer value that was extracted, or @@ -805,8 +807,10 @@ /// /// @param[in] bitfield_bit_offset /// The bit offset of the bitfield value in the extracted -/// integer (the number of bits to shift the integer to the -/// right). +/// integer. For little-endian data, this is the offset of +/// the LSB of the bitfield from the LSB of the integer. +/// For big-endian data, this is the offset of the MSB of the +/// bitfield from the MSB of the integer. /// /// @return /// The signed bitfield integer value that was extracted, or Index: lldb/trunk/unittests/Core/DataExtractorTest.cpp === --- lldb/trunk/unittests/Core/DataExtractorTest.cpp +++ lldb/trunk/unittests/Core/DataExtractorTest.cpp @@ -0,0 +1,39 @@ +//===-- DataExtractorTest.cpp ---*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// + +#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0) +// Workaround for MSVC standard library bug, which fails to include when +// exceptions are disabled. +#include +#endif + +#include "gtest/gtest.h" + +#include "lldb/Core/DataExtractor.h" + +using namespace lldb_private; + +TEST(DataExtractorTest, GetBitfield) +{ +char buffer[] = { 0x01, 0x23, 0x45, 0x67 }; +DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle, sizeof(void *)); +DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *)); + +lldb::offset_t offset; + +offset = 0; +ASSERT_EQ(buffer[1], LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); +offset = 0; +ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); + +offset = 0; +ASSERT_EQ(buffer[1], LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8)); +offset = 0; +ASSERT_EQ(buffer[1], BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8)); +} Index: lldb/trunk/unittests/Core/CMakeLists.txt === --- lldb/trunk/unittests/Core/CMakeLists.txt +++ lldb/trunk/unittests/Core/CMakeLists.txt @@ -1,3 +1,4 @@ add_lldb_unittest(LLDBCoreTests + DataExtractorTest.cpp ScalarTest.cpp ) Index: lldb/trunk/source/Core/ValueObject.cpp === --- lldb/trunk/source/Core/ValueObject.cpp +++ lldb/trunk/source/Core/ValueObject.cpp @@ -2146,15 +2146,19 @@ synthetic_child_sp = GetSyntheticChild (index_const_str); if (!synthetic_child_sp) { +uint32_t bit_field_size = to - from + 1; +uint32_t bit_field_offset = from; +if (GetDataExtractor().GetByteOrder() == eByteOrderBig) +bit_field_offset = GetByteSize() * 8 - bit_field_size - bit_field_offset; // We haven't made a synthetic array member for INDEX yet, so // lets make one and cache it for any future reference. ValueObjectChild *synthetic_child = new ValueObjectChild (*this, GetCompilerType(), index_const_str, GetByteSize(), 0, -
Re: [Lldb-commits] [PATCH] D18982: Handle bit fields on big-endian systems correctly
uweigand updated this revision to Diff 53605. uweigand added a comment. Updated interface documentation in DataExtractor.h. Added unit test case for the DataExtractor::GetMaxU64Bitfield and GetMaxS64Bitfield routines. Retested on System z and Intel. http://reviews.llvm.org/D18982 Files: include/lldb/Core/DataExtractor.h source/Core/DataExtractor.cpp source/Core/ValueObject.cpp unittests/Core/CMakeLists.txt unittests/Core/DataExtractorTest.cpp Index: unittests/Core/DataExtractorTest.cpp === --- unittests/Core/DataExtractorTest.cpp +++ unittests/Core/DataExtractorTest.cpp @@ -0,0 +1,39 @@ +//===-- DataExtractorTest.cpp ---*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// + +#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0) +// Workaround for MSVC standard library bug, which fails to include when +// exceptions are disabled. +#include +#endif + +#include "gtest/gtest.h" + +#include "lldb/Core/DataExtractor.h" + +using namespace lldb_private; + +TEST(DataExtractorTest, GetBitfield) +{ +char buffer[] = { 0x01, 0x23, 0x45, 0x67 }; +DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle, sizeof(void *)); +DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *)); + +lldb::offset_t offset; + +offset = 0; +ASSERT_EQ(buffer[1], LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); +offset = 0; +ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); + +offset = 0; +ASSERT_EQ(buffer[1], LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8)); +offset = 0; +ASSERT_EQ(buffer[1], BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8)); +} Index: unittests/Core/CMakeLists.txt === --- unittests/Core/CMakeLists.txt +++ unittests/Core/CMakeLists.txt @@ -1,3 +1,4 @@ add_lldb_unittest(LLDBCoreTests + DataExtractorTest.cpp ScalarTest.cpp ) Index: source/Core/ValueObject.cpp === --- source/Core/ValueObject.cpp +++ source/Core/ValueObject.cpp @@ -2146,15 +2146,19 @@ synthetic_child_sp = GetSyntheticChild (index_const_str); if (!synthetic_child_sp) { +uint32_t bit_field_size = to - from + 1; +uint32_t bit_field_offset = from; +if (GetDataExtractor().GetByteOrder() == eByteOrderBig) +bit_field_offset = GetByteSize() * 8 - bit_field_size - bit_field_offset; // We haven't made a synthetic array member for INDEX yet, so // lets make one and cache it for any future reference. ValueObjectChild *synthetic_child = new ValueObjectChild (*this, GetCompilerType(), index_const_str, GetByteSize(), 0, - to-from+1, - from, + bit_field_size, + bit_field_offset, false, false, eAddressTypeInvalid, Index: source/Core/DataExtractor.cpp === --- source/Core/DataExtractor.cpp +++ source/Core/DataExtractor.cpp @@ -733,8 +733,11 @@ uint64_t uval64 = GetMaxU64 (offset_ptr, size); if (bitfield_bit_size > 0) { -if (bitfield_bit_offset > 0) -uval64 >>= bitfield_bit_offset; +int32_t lsbcount = bitfield_bit_offset; +if (m_byte_order == eByteOrderBig) +lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; +if (lsbcount > 0) +uval64 >>= lsbcount; uint64_t bitfield_mask = ((1ul << bitfield_bit_size) - 1); if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64) return uval64; @@ -749,8 +752,11 @@ int64_t sval64 = GetMaxS64 (offset_ptr, size); if (bitfield_bit_size > 0) { -if (bitfield_bit_offset > 0) -sval64 >>= bitfield_bit_offset; +int32_t lsbcount = bitfield_bit_offset; +if
Re: [Lldb-commits] [PATCH] D18982: Handle bit fields on big-endian systems correctly
labath added a subscriber: labath. labath added a comment. It would be worthwhile to add a unit test for the DataExtractor fix (we don't have many of those, but we're trying to build them up). http://reviews.llvm.org/D18982 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D18982: Handle bit fields on big-endian systems correctly
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good as long as all tests still pass on all other systems. http://reviews.llvm.org/D18982 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D18982: Handle bit fields on big-endian systems correctly
uweigand created this revision. uweigand added reviewers: granata.enrico, clayborg. uweigand added a subscriber: lldb-commits. Currently, the DataExtractor::GetMaxU64Bitfield and GetMaxS64Bitfield routines assume the incoming "bitfield_bit_offset" parameter uses little-endian bit numbering, i.e. a bitfield_bit_offset 0 refers to a bitfield whose least-significant bit coincides with the least- significant bit of the surrounding integer. On many big-endian systems, however, the big-endian bit numbering is used for bit fields. Here, a bitfield_bit_offset 0 refers to a bitfield whose most-significant bit conincides with the most- significant bit of the surrounding integer. Now, in principle LLDB could arbitrarily choose which semantics of bitfield_bit_offset to use. However, there are two problems with the current approach: - When parsing DWARF, LLDB decodes bit offsets in little-endian bit numbering on LE systems, but in big-endian bit numbering on BE systems. Passing those offsets later on into the DataExtractor routines gives incorrect results on BE. - In the interim, LLDB's type layer combines byte and bit offsets into a single number. I.e. instead of recording bitfields by specifying the byte offset and byte size of the surrounding integer *plus* the bit offset of the bit field within that field, it simply records a single bit offset number. Now, note that converting from byte offset + bit offset to a single offset value and back is well-defined if we either use little-endian byte order *and* little-endian bit numbering, or use big-endian byte order *and* big-endian bit numbering. Any other combination will yield incorrect results. Therefore, the simplest approach would seem to be to always use the bit numbering that matches the system byte order. This makes storing a single bit offset valid, and makes the existing DWARF code correct. The only place to fix is to teach DataExtractor to use big-endian bit numbering on big endian systems. However, there is only additional caveat: we also get bit offsets from LLDB synthetic bitfields. While the exact semantics of those doesn't seem to be well-defined, from test cases it appears that the intent was for the user-provided synthetic bitfield offset to always use little-endian bit numbering. Therefore, on a big-endian system we now have to convert those to big-endian bit numbering to remain consistent. http://reviews.llvm.org/D18982 Files: source/Core/DataExtractor.cpp source/Core/ValueObject.cpp Index: source/Core/ValueObject.cpp === --- source/Core/ValueObject.cpp +++ source/Core/ValueObject.cpp @@ -2146,15 +2146,19 @@ synthetic_child_sp = GetSyntheticChild (index_const_str); if (!synthetic_child_sp) { +uint32_t bit_field_size = to - from + 1; +uint32_t bit_field_offset = from; +if (GetDataExtractor().GetByteOrder() == eByteOrderBig) +bit_field_offset = GetByteSize() * 8 - bit_field_size - bit_field_offset; // We haven't made a synthetic array member for INDEX yet, so // lets make one and cache it for any future reference. ValueObjectChild *synthetic_child = new ValueObjectChild (*this, GetCompilerType(), index_const_str, GetByteSize(), 0, - to-from+1, - from, + bit_field_size, + bit_field_offset, false, false, eAddressTypeInvalid, Index: source/Core/DataExtractor.cpp === --- source/Core/DataExtractor.cpp +++ source/Core/DataExtractor.cpp @@ -733,8 +733,11 @@ uint64_t uval64 = GetMaxU64 (offset_ptr, size); if (bitfield_bit_size > 0) { -if (bitfield_bit_offset > 0) -uval64 >>= bitfield_bit_offset; +int32_t lsbcount = bitfield_bit_offset; +if (m_byte_order == eByteOrderBig) +lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; +if (lsbcount > 0) +uval64 >>= lsbcount; uint64_t bitfield_mask = ((1ul << bitfield_bit_size) - 1); if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64)