This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Avoid implicit memset in hugify
AbandonedPublic

Authored by yavtuk on Jul 1 2022, 12:19 AM.

Details

Summary

This patch removes memset usage from C library
relative to https://reviews.llvm.org/D128960 commit

Diff Detail

Event Timeline

yavtuk created this revision.Jul 1 2022, 12:19 AM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ayermolo. · View Herald Transcript
yavtuk requested review of this revision.Jul 1 2022, 12:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 12:19 AM
Amir accepted this revision.Jul 1 2022, 12:40 AM

Thanks for fixing it!

This revision is now accepted and ready to land.Jul 1 2022, 12:40 AM
Amir added a comment.Jul 1 2022, 12:50 AM

Before landing, please retitle the diff to e.g. "[BOLT] Avoid implicit memset in hugify", and amend the commit message to reflect the change.

yavtuk retitled this revision from fix for user-func-reorder.c test to [BOLT] Avoid implicit memset in hugify.Jul 1 2022, 1:00 AM
yavtuk updated this revision to Diff 441629.Jul 1 2022, 1:08 AM

updated commit message

yavtuk updated this revision to Diff 441630.Jul 1 2022, 1:11 AM

@Amir Thanks for the review

yota9 added a comment.Jul 1 2022, 1:23 AM

Just curious is there any reason to memset this buffer? It will be overwritten by read anyway, probably buf[0] = '\0' is more then enough here. And looks like the size might be truncated to smth like 32-64, what do you think?

bolt/runtime/hugify.cpp
39

This is OOS but sizeof(buf) if you don't mind

yavtuk added a comment.Jul 1 2022, 1:32 AM

Just curious is there any reason to memset this buffer? It will be overwritten by read anyway, probably buf[0] = '\0' is more then enough here. And looks like the size might be truncated to smth like 32-64, what do you think?

I agree with __read, I am going to suggest a patch with Hugify support for PIE binaries soon, and this commit contains sizeof(buf).

buf[0] = '\0' vs memSet
In my thoughts both options are available here because it doesn’t affect performance

maksfb accepted this revision.Jul 5 2022, 2:56 PM

Thanks for the fix.