This is an archive of the discontinued LLVM Phabricator instance.

MC: Remove redundant substr() call
ClosedPublic

Authored by sbc100 on May 17 2018, 6:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.May 17 2018, 6:33 PM
dschuff accepted this revision.May 29 2018, 4:42 PM
This revision is now accepted and ready to land.May 29 2018, 4:42 PM
This revision was automatically updated to reflect the committed changes.

This somehow broke a stage-2 GreenDragon: http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/10387/ (it might not be the issue, though)

I noticed one small issue with the code but I'm not sure if any callers rely on that. Comment below.

thegameg added inline comments.May 30 2018, 8:50 AM
llvm/trunk/lib/Object/MachOObjectFile.cpp
110

.substr never goes past end if Offset > getData().size(). Maybe add an assert to make sure no callers rely on this?

sbc100 added inline comments.May 30 2018, 9:10 AM
llvm/trunk/lib/Object/MachOObjectFile.cpp
110

Good point, I was trying to figure out what the original intention was and I guess that may have been it. But it doesn't make a lot of sense to me.. from looking at the StringRef::substr code it looks like it will always return a pointer to the end of the string if you request and out of bounds offset, then the code the receives this pointer will read out of bounds just off the end of the string. I can't see why this would be useful or desired. But adding an assert would certainly be an improvement. I will do that.

Do you think this change could be responsible for the build break you are seeing? I can't really see how it could break anything that wasn't already very broken. But I could be missing something.

thegameg added inline comments.May 30 2018, 9:14 AM
llvm/trunk/lib/Object/MachOObjectFile.cpp
110

Thanks! For the build, nothing obvious comes to mind, once I'll be able to reproduce locally I'll let you know.

thegameg added inline comments.May 30 2018, 9:49 AM
llvm/trunk/lib/Object/MachOObjectFile.cpp
110

Nope, reverted this locally and I can still see the fail. Sorry for the false alarm.