LGTM

Perhaps simplify the test a little by requesting asm output and checking that the section directives are correct.

LGTM

In D37789#869660, @tejohnson wrote:

LGTM, not sure how we didn't hit this before!

Short of restructuring the class hierarchy so that AliasSummary doesn't have the refs member

LGTM, thanks!

Before considering this patch I would first like to see another client of the function that you are adding.

Address review comments.

In D37655#867084, @dblaikie wrote:

Reckon maybe a test case with two units - one using gnu pubnames, one not, would be worth having to demonstrate that all works fine & is appropriately respected?

LGTM

Ping.

Ping.

Thanks. Once we apply this fix to other platforms this would seem to fix PR17633.

In D36351#864001, @pcc wrote:

In D36351#863966, @ruiu wrote:

I have a local patch to AutoFDO that I use to create profiles. I can upload it somewhere if you'd like.

Thanks. But the paper says that they used perf command to get profiling data. Why did you have to use AutoFDO?

As far as I know the missing piece is a tool that converts LBR data collected by perf into a weighted call graph. AutoFDO already has support for creating weighted call graphs from perf LBR data, so I just needed to teach it to output the graph in a different format.

In D36351#863966, @ruiu wrote:

I have a local patch to AutoFDO that I use to create profiles. I can upload it somewhere if you'd like.

Thanks. But the paper says that they used perf command to get profiling data. Why did you have to use AutoFDO?

In D36351#863961, @ruiu wrote:

How can I test this? I wonder how to create profiling data to feed this feature.

In D37407#861548, @eugenis wrote:

Ok, looks fine to me.
Could you elaborate on the linker issue - is it that some of the uses of these constants can not be represented with existing relocation types? Or is it missing functionality in the existing linkers?

In D37410#859518, @zturner wrote:

I think you actually can rename open files on Windows. Or at least, you can try to. I'll have to dig in when I have a bit more time though to see if there's any weird things we need to be aware of when doing it, or casse where it won't work.

But basically, what you do is call SetFileInformationByHandle and set the FileInformationClass parameter to FileRenameInfo, and fill out a FILE_RENAME_INFO structure to pass for the 3rd argument.

LGTM

I'm not sure if this change will do what you think it does. The linker should create GOTs and PLTs for all symbols, even those defined by .s files, as long as an appropriate relocation exists. In particular the change you are making in D37373 should on its own result in the creation of PLT entries for __xray_CustomEntry. Can you please explain the problem that you are trying to solve?

LGTM

LGTM

In D34560#855831, @rnk wrote:

In D34560#855735, @pcc wrote:

I thought that the purpose of this change was to make thinlto use LLVM_APPEND_VC_REV to control whether to add the VCS revision to the hash. Or do you think there should be a separate CMake variable to control whether thinlto uses the VCS revision?

I think I see what happened now.

LLVM_APPEND_VC_REV was a dormant option that probably nobody was using. The effect of that option was to add the revision to LLVM_VERSION_STRING in llvm-config.h, which causes llvm-config.h to change after every git commit. This is, IMO, completely undesirable.

Because that functionality was disabled by default, you and others (Jordan?) created VCSRevision.h and the things that came before it to compute accurate version information as part of the build. Rafael didn't want actions to run after changing git branches, so he wrote this patch to put the behavior under this flag. Because you requested the default to change, now everyone is getting the unintended updates to llvm-config.h, causing much larger rebuilds.

However, nobody noticed for months, because cmake caches option settings when it builds. In order to observe the new LLVM_APPEND_VC_REV default value, you have to delete CMakeCache.txt.

In D34560#855667, @rnk wrote:

In D34560#789101, @pcc wrote:

Using LLVM_APPEND_VC_REV seems fine to me, but we probably want to update the documentation and most likely change the default because right now the VC revision affects ThinLTO cache invalidation, and we shouldn't silently use stale cache entries when clang is upgraded.

@beanz any thoughts?

I think we can revert the change to the default value of LLVM_APPEND_VC_REV. The VCS revision doesn't need to be included in LLVM_VERSION_STRING because ThinLTO includes VCSRevision.h to add LLVM_REVISION into the hash.