This is an archive of the discontinued LLVM Phabricator instance.

[libc] Align src buffer instead of dst buffer
ClosedPublic

Authored by gchatelet on Dec 17 2020, 5:58 AM.

Details

Summary

We used to align destination buffer instead of source buffer for the loop of block copy.
This is a mistake.

Diff Detail

Event Timeline

gchatelet created this revision.Dec 17 2020, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 5:58 AM
gchatelet requested review of this revision.Dec 17 2020, 5:58 AM
gchatelet edited reviewers, added: courbet; removed: avieira, sivachandra.Jan 5 2021, 6:55 AM
gchatelet added subscribers: avieira, sivachandra.

Hi Guillaume,

Sorry for the late reply, I was looking at some other things and then holidays happened. I started looking at making some changes to CopyAlignedBlocks just like we discussed earlier. One of them is this one and I noticed this makes the MemcpyUtilsTest.CopyAlignedBlocks fail in libc_string_unittests. Specifically the ones that test destination misalignment. It seems the testing infrastructure keeps track of what bytes were read and written. I checked and making the source the misaligned input and switching the read and written expected traces makes it passs. Which makes sense I guess.

Would be good to make the change at the same time though.

gchatelet updated this revision to Diff 314825.Jan 6 2021, 2:08 AM
  • Address comments and rebase

Hi Guillaume,

Sorry for the late reply, I was looking at some other things and then holidays happened. I started looking at making some changes to CopyAlignedBlocks just like we discussed earlier. One of them is this one and I noticed this makes the MemcpyUtilsTest.CopyAlignedBlocks fail in libc_string_unittests. Specifically the ones that test destination misalignment. It seems the testing infrastructure keeps track of what bytes were read and written. I checked and making the source the misaligned input and switching the read and written expected traces makes it passs. Which makes sense I guess.

Would be good to make the change at the same time though.

Thx for noticing, it should be good now. I'll move you back to reviewers if you don't mind.

gchatelet removed a subscriber: avieira.

Sure, LGTM. What is the usual convention for Libc reviewing? Should I 'accept revision' if I agree with it even though I'm not a maintainer?

courbet accepted this revision.Jan 6 2021, 2:44 AM
This revision is now accepted and ready to land.Jan 6 2021, 2:44 AM

Sure, LGTM. What is the usual convention for Libc reviewing? Should I 'accept revision' if I agree with it even though I'm not a maintainer?

I don't know TBH :-)
I think it's fine if you accept revision even if you're not a maintainer.

This revision was landed with ongoing or failed builds.Jan 6 2021, 4:05 AM
This revision was automatically updated to reflect the committed changes.