Rebased over https://reviews.llvm.org/D158122. Addressed the comments on the test code.

Moved the test to test/CodeGen and removed the -filetype=asm flag.

In D155732#4588492, @compnerd wrote:

I think that I generally prefer this approach. I would however like to see a test for the storage class changes (and ideally split from this change - it fixes a problem needed to fix here, but is a completely standalone improvement).

In D157059#4572809, @Michael137 wrote:

Hi, this is failing on swift-ci (runs on x86 bots) with the following error:

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/build/Ninja-ReleaseAssert+stdlib-Release/lldb-macosx-x86_64/unittests/ObjectFile/PECOFF/./ObjectFilePECOFFTests --gtest_filter=SectionSizeTest.NoAlignmentPadding
--
YAML:12:5: error: unknown key 'SizeOfRawData'
    SizeOfRawData:   512
    ^~~~~~~~~~~~~
/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49: Failure
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

/Users/ec2-user/jenkins/workspace/oss-lldb-incremental-macos-cmake/llvm-project/lldb/unittests/ObjectFile/PECOFF/TestSectionSize.cpp:49
Value of: llvm::detail::TakeExpected(ExpectedFile)
Expected: succeeded
  Actual: failed  (convertYAML() failed)

https://ci.swift.org/view/LLDB/job/oss-lldb-incremental-macos-cmake/

Could you take a look please?

Updated to treat all private linkage symbols as internal and add them to the symbol table, as Reid suggested. This fixes the issue with private linkage and the swift issue.

Please take another look. @efriedma

Update to address comments.

Please take another look.

Can you clarify? Does it mean that we need to check A.isInSection() && Layout.getSymbolOffset(A) < getSectionAddressSize(&A.getSection()) before we create a new symbol at where symbol A is? What should happen if that's not the case?

Something like that, yes. If it fails, we need to print an error, because we don't have any way to encode it. At least, I don't think we have a way to encode it? Not sure off the top of my head what offsets the linker would tolerate.

In D155732#4523640, @efriedma wrote:

In D155732#4521021, @hjyamauchi wrote:

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

If you mean like the following, that would unintentionally trigger when we drop the relocation in the newly-added code (in the if (MCSec == &A.getSection()) case) because IsResolved is false.

case AArch64::fixup_aarch64_pcrel_branch26:
case AArch64::fixup_aarch64_pcrel_call26:
  if (TheTriple.isOSBinFormatCOFF() && !IsResolved && SignedValue != 0) {
    // MSVC link.exe and lld do not support this relocation type with a non-zero offset
    Ctx.reportError(Fixup.getLoc(), "fixup value not supported by the linkers");
  }

I still don't understand why the MCSec == &A.getSection() case is relevant; if the offset is constant, it should be resolved, since "resolved" means "doesn't need a relocation".

In D155732#4521023, @hjyamauchi wrote:

There's a bunch of code for A.isTemporary() a few lines earlier; would it make sense to integrate with that? This is sort of similar to what we do with OffsetSymbols, except the maximum offset is zero.

That may make sense as long as it's okay to check if the relocation type is fixup_aarch64_pcrel_branch26 or fixup_aarch64_pcrel_call26 before the getRelocType() call.

Updated the revision with the better test and changes per review comments.

In D155732#4522178, @compnerd wrote:

Thanks for the test transcript! It does indeed look like link.exe ignores the existing offset. I also independently tested this, and it actually looks like link.exe blindly ors the target offset bits on top of the existing opcode, just like lld does: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.6/lld/COFF/Chunks.cpp#L306-L322 Depending on the contents of the original offset and the intended symbol difference, the offset may end up being partially visible or not. In any case - the linker doesn't indeed seem to be able to handle immediate offsets here.

You can't create symbols at arbitrary offsets from the start of a section; you need to check that the symbol actually points somewhere within the section.

There's a bunch of code for A.isTemporary() a few lines earlier; would it make sense to integrate with that? This is sort of similar to what we do with OffsetSymbols, except the maximum offset is zero.

We should teach adjustFixupValue() in AArch64AsmBackend.cpp to print an error if an unresolved branch with an offset gets there somehow.

In D155732#4519183, @compnerd wrote:

In D155732#4516320, @mstorsjo wrote:

In D155732#4515825, @hjyamauchi wrote:

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Yeah. To be clearer, what I observed was that the relocation type works as long as the offset that's embedded in the instruction is zero (I suspect that the linker doesn't use the offset in relocation or assumes it's zero.) I also tried a small local patch in lld to take this non-zero offset into account in the relocation and it also avoided the crashes I'm seeing.

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

I would agree, but it seems that link also does not support the relocation with addend, and I would rather that we retain compatibility with the reference implementation. It sounds like it is only the addend form that we would need to avoid on Windows, not the general relocation.

I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.

In D155732#4516410, @efriedma wrote:

That sounds like the correct thing to do; if a relocation with an offset isn’t supported by lld (but it is by link.exe), we should fix the linker instead of trying to avoid generating it.

The description sounds like it's also not supported by link.exe? Not sure.

In D155732#4515655, @compnerd wrote:

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format indicates that there does exist such a relocation:

IMAGE_REL_ARM64_BRANCH26 0x0003 The 26-bit relative displacement to the target, for B and BL instructions.

If this relocation is unsupported, we should just disable that relocation from being formed or we should figure out what the linker expectations are.

Added a CHECK-NOT.

In D150305#4336273, @hjyamauchi wrote:

In D150305#4336082, @MaskRay wrote:

In D150305#4335987, @hjyamauchi-test wrote:

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

Done! Matching the GitHub account is a good reason and I know others renaming for this reason.

Renamed to hjyamauchi to hjyamauchi-test and renamed yamauchi to hjyamauchi :)
@hjyamauchi contains all the contribution history (all occurrences of @yamauchi in old messages are replaced with @hjyamauchi).

If the result looks good, I can disable hjyamauchi-test so that people will not pick the wrong user.
(Like that I had @maskray0 which has been disabled.)

Thanks! I'll see how the new 'hjyamauchi' works out and let you know. I'll also see if I can integrate it with my github login.

In D150305#4336082, @MaskRay wrote:

In D150305#4335987, @hjyamauchi-test wrote:

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

Done! Matching the GitHub account is a good reason and I know others renaming for this reason.

Renamed to hjyamauchi to hjyamauchi-test and renamed yamauchi to hjyamauchi :)
@hjyamauchi contains all the contribution history (all occurrences of @yamauchi in old messages are replaced with @hjyamauchi).

If the result looks good, I can disable hjyamauchi-test so that people will not pick the wrong user.
(Like that I had @maskray0 which has been disabled.)

In D149800#4330831, @davidxl wrote:

This patch uses a cleaner method than the previous effort. There is some differences:

1. yamauchi's patch works for both iFDO and autoFDO, while this patch limits to iFDO
2. yamauchi's patch also handles size optimization (IIRC) at granularity smaller than function

Longer term, the previous functionality should fold into the new framework.

Late to this, but LGTM.

Address comment.

Okay added a new test.

Address comment.

In D97592#2593258, @davidxl wrote:

Perhaps add a test case showing that up to 3 ranges can be promoted?

D93281 looks good to me.

I can add "cl::ZeroOrMore" to "-vp-counters-per-site". Would it be good?

In D92669#2437383, @davidxl wrote:

The size increase is significant. It is likely that some very large apps may run into size limit issue at link time with this change.

Adjust only for dynamic linking mode. Rebase.

In D92669#2434247, @davidxl wrote:

how much size increase is in clang instrumented build?

Address comment.

https://reviews.llvm.org/D92669 should fix the issue.

Uploaded https://reviews.llvm.org/D92599

Hi, we encountered this issue https://bugs.llvm.org/show_bug.cgi?id=48055 which reverting this patch seems to make it go away.

This is with clang trying to PGO itself. Build with -DLLVM_BUILD_INSTRUMENTED=IR -DLLVM_BUILD_RUNTIME=No and use the result to build the LLVM tree again.

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318445929&repo=ash&lineNumber=17322

In D81682#2328239, @dmajor wrote:

In D81682#2168134, @yamauchi wrote:

In D81682#2168028, @dmajor wrote:

The re-landing of this patch in 4a539faf74b9b4c25ee3b880e4007564bd5139b0 causes our PGO build of clang to have thousands of

LLVM Profile Warning: Unable to track new values: Running out of static counters.  Consider using option -mllvm -vp-counters-per-site=<n> to allocate more value profile counters at compile time.

Is that expected? Should the cmake set a higher value of -vp-counters-per-site in self-hosted PGO builds?

This has been reverted for now. There was a related issue.

Our build bots still show lots of this warning, is that expected?

Update the patch description.

I'm not familiar with the history of this file well, but I had the same impression: it'd be nice if there's a single copy but since compiler-rt is set up to build without depending on llvm/, it's in the way it is currently.

Rebase.