This is an archive of the discontinued LLVM Phabricator instance.

Differential D136067

[memprof] Support installation of memprof headers
ClosedPublic

Authored by Enna1 on Oct 17 2022, 4:05 AM.

Details

Reviewers
tejohnson
snehasish
Commits
rG30f6f2c09d6f: [memprof] Support installation of memprof headers
Summary

This change allows users manually calling memprof public C API (e.g. __memprof_profile_dump).

Diff Detail

Event Timeline

Enna1 created this revision.Oct 17 2022, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 4:05 AM
Harbormaster completed remote builds in B192463: Diff 468155.Oct 17 2022, 4:22 AM
Enna1 edited the summary of this revision. (Show Details)Oct 17 2022, 4:22 AM
Enna1 added reviewers: tejohnson, snehasish.
Enna1 added a subscriber: MTC.
Enna1 published this revision for review.Oct 17 2022, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 4:32 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
tejohnson added inline comments.Oct 17 2022, 10:58 AM
compiler-rt/include/CMakeLists.txt
90

What are the implications of putting both of these files in the sanitizer subdirectory? I notice that one of them normally lives under profile/. I guess that file (MemProfData.inc) probably doesn't matter as it won't be called by user code?

Enna1 added inline comments.Oct 17 2022, 8:13 PM
compiler-rt/include/CMakeLists.txt
90

I notice that one of them normally lives under profile/. I guess that file (MemProfData.inc) probably doesn't matter as it won't be called by user code?

Yes, MemProfData.inc lives under profile/ directory, and this file won't be called by user code.

memprof_interface.h is the one that matters. Currently in user code, #include <sanitizer/memprof_interface.h> will cause a compilation failed due to memprof_interface.h not found.

Maybe we should only install memprof_interface.h to sanitizer/ directory? WDYT?

tejohnson added inline comments.Oct 18 2022, 8:27 AM
compiler-rt/include/CMakeLists.txt
90

Yeah only installing the interface header might be best for now. If it turns out we need MemProfData.inc installed in the future we can add that and it will be helpful to understand the context of that need so we know where to install it.

Enna1 updated this revision to Diff 468759.Oct 18 2022, 6:52 PM

Address review comments

Harbormaster completed remote builds in B192894: Diff 468759.Oct 18 2022, 7:10 PM
tejohnson accepted this revision.Oct 18 2022, 9:01 PM

lgtm

This revision is now accepted and ready to land.Oct 18 2022, 9:01 PM
Closed by commit rG30f6f2c09d6f: [memprof] Support installation of memprof headers (authored by Enna1). · Explain WhyOct 20 2022, 5:34 AM
This revision was automatically updated to reflect the committed changes.
Enna1 added a commit: rG30f6f2c09d6f: [memprof] Support installation of memprof headers.
mgorny added a subscriber: mgorny.Oct 23 2022, 4:56 AM

Could you please fix this to respect COMPILER_RT_BUILD_MEMPROF? We build separate packages for different compiler-rt components, and now memprof headers end up in every one of them, causing file collision errors.

compiler-rt/include/CMakeLists.txt
87

This causes memprof headers to be installed even if COMPILER_RT_BUILD_MEMPROF is disabled.

Enna1 mentioned this in D136550: [memprof] Respect COMPILER_RT_BUILD_MEMPROF when install memprof headers.Oct 23 2022, 5:42 AM
In D136067#3877596, @mgorny wrote:

Could you please fix this to respect COMPILER_RT_BUILD_MEMPROF? We build separate packages for different compiler-rt components, and now memprof headers end up in every one of them, causing file collision errors.

send https://reviews.llvm.org/D136550 to fix this.

mgorny added a comment.Oct 23 2022, 5:52 AM

Thank you!

Revision Contents

PathSize
compiler-rt/
include/
5 lines

Diff 469187

compiler-rt/include/CMakeLists.txt

Show First 20 Lines • Show All 77 Lines • ▼ Show 20 Linesinstall(FILES ${SANITIZER_HEADERS}
COMPONENT compiler-rt-headers COMPONENT compiler-rt-headers
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/sanitizer) DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/sanitizer)
# Install fuzzer headers. # Install fuzzer headers.
install(FILES ${FUZZER_HEADERS} install(FILES ${FUZZER_HEADERS}
COMPONENT compiler-rt-headers COMPONENT compiler-rt-headers
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/fuzzer) DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/fuzzer)
# Install memprof headers.
install(FILES sanitizer/memprof_interface.h
mgornyUnsubmitted
Not Done
ReplyInline Actions

This causes memprof headers to be installed even if COMPILER_RT_BUILD_MEMPROF is disabled.

mgorny: This causes memprof headers to be installed even if `COMPILER_RT_BUILD_MEMPROF` is disabled.
COMPONENT compiler-rt-headers
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/sanitizer)
tejohnsonUnsubmitted
Not Done
ReplyInline Actions

What are the implications of putting both of these files in the sanitizer subdirectory? I notice that one of them normally lives under profile/. I guess that file (MemProfData.inc) probably doesn't matter as it won't be called by user code?

tejohnson: What are the implications of putting both of these files in the sanitizer subdirectory? I…
Enna1AuthorUnsubmitted
Done
ReplyInline Actions

I notice that one of them normally lives under profile/. I guess that file (MemProfData.inc) probably doesn't matter as it won't be called by user code?

Yes, MemProfData.inc lives under profile/ directory, and this file won't be called by user code.

memprof_interface.h is the one that matters. Currently in user code, #include <sanitizer/memprof_interface.h> will cause a compilation failed due to memprof_interface.h not found.

Maybe we should only install memprof_interface.h to sanitizer/ directory? WDYT?

Enna1: > I notice that one of them normally lives under profile/. I guess that file (MemProfData.inc)…
tejohnsonUnsubmitted
Not Done
ReplyInline Actions

Yeah only installing the interface header might be best for now. If it turns out we need MemProfData.inc installed in the future we can add that and it will be helpful to understand the context of that need so we know where to install it.

tejohnson: Yeah only installing the interface header might be best for now. If it turns out we need…
# Install xray headers. # Install xray headers.
install(FILES ${XRAY_HEADERS} install(FILES ${XRAY_HEADERS}
COMPONENT compiler-rt-headers COMPONENT compiler-rt-headers
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/xray) DESTINATION ${COMPILER_RT_INSTALL_INCLUDE_DIR}/xray)
# Install ORC headers. # Install ORC headers.
install(FILES ${ORC_HEADERS} install(FILES ${ORC_HEADERS}
COMPONENT compiler-rt-headers COMPONENT compiler-rt-headers
Show All 23 Lines