This is an archive of the discontinued LLVM Phabricator instance.

Fix the Endianness bug by adding the little endian UTF marker.
ClosedPublic

Authored by ecbeckmann on May 9 2017, 11:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ecbeckmann created this revision.May 9 2017, 11:46 AM
uweigand edited edge metadata.May 9 2017, 11:57 AM

This doesn't look correct either:

EndianCorrectedNameString[0] = 0xFEFF;

On a big-endian host system, this will set the endian marker to FE FF, i.e. to indicate BE instead of LE.

Probably needs a ulittle16 somewhere to work on both big- and little-endian hosts.

zturner added inline comments.May 9 2017, 12:04 PM
llvm/tools/llvm-readobj/COFFDumper.cpp
1565–1572 ↗(On Diff #98331)

How about this:

ArrayRef<UTF16> RawEntryNameString = unwrapOrError(RSF.getEntryNameString(Entry));
std::vector<UTF16> EndianCorrectedString;
if (llvm::sys::IsBigEndianHost) {
  EndianCorrectedString.resize(RawEntryNameString.size() + 1);
  std::copy(RawEntryNameString.begin(), RawEntryNameString.end(), EndianCorrectedString.begin() + 1);
  EndianCorrectedString[0] = UNI_UTF16_BYTE_ORDER_MARK_SWAPPED;
  RawEntryNameString = makeArrayRef(EndianCorrectedString);
}
ruiu added a subscriber: ruiu.May 9 2017, 12:06 PM

Adding a byte order mark doesn't seems like a good way to fix the issue. Why don't you add a flag indicating whether it is BE or LE to convertUTF16ToUTF8String?

ecbeckmann updated this revision to Diff 98337.May 9 2017, 12:12 PM

Check endianness of system before adding marker.

ecbeckmann marked an inline comment as done.May 9 2017, 12:13 PM
uweigand added inline comments.May 9 2017, 12:14 PM
llvm/tools/llvm-readobj/COFFDumper.cpp
1565–1572 ↗(On Diff #98331)

This does fix the problem for me.

ruiu added a comment.May 9 2017, 12:32 PM

Do you mean the bot is still red? If I broke something I'd first try to roll it back if the error is not just a simple mistake but needs another round of code review.

zturner accepted this revision.May 9 2017, 12:47 PM
This revision is now accepted and ready to land.May 9 2017, 12:47 PM
This revision was automatically updated to reflect the committed changes.