Skip to content

[Windows] Add support for emitting PGO/LTO magic strings in the Windows PE debug directory #114260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[Windows] Add support for emitting PGO/LTO magic strings in the Windows PE debug directory #114260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f903e7e
Correct test
mikolaj-pirog Oct 30, 2024
9dfc603
Add emitting of Windows PE PGO/LTO strings
mikolaj-pirog Oct 30, 2024
aff0c90
Correct test description
mikolaj-pirog Oct 30, 2024
5cb0237
Don't write to DwoOnly files
mikolaj-pirog Nov 4, 2024
0e3749f
Apply reviewers' suggestions to testing
mikolaj-pirog Nov 4, 2024
c98e758
LTO tests
mikolaj-pirog Nov 5, 2024
8f2b91a
Pgu test
mikolaj-pirog Nov 5, 2024
46e70ab
Remove MSVC from target triple
mikolaj-pirog Nov 5, 2024
a5bd283
Consolidate tests to one
mikolaj-pirog Nov 5, 2024
d999486
Formatting
mikolaj-pirog Nov 5, 2024
adb3b39
Formatting
mikolaj-pirog Nov 5, 2024
5c0477c
Add necessary requires to tests
mikolaj-pirog Nov 7, 2024
1f2a088
Remove aarch testing
mikolaj-pirog Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/BackendUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,9 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
Options.MCOptions.PPCUseFullRegisterNames =
CodeGenOpts.PPCUseFullRegisterNames;
Options.MisExpect = CodeGenOpts.MisExpect;
Options.MCOptions.PgoInstrumentation = CodeGenOpts.getProfileInstr() > 0;
Options.MCOptions.PgoUse =
CodeGenOpts.getProfileUse() > 0 || !CodeGenOpts.SampleProfileFile.empty();

return true;
}
Expand Down
23 changes: 23 additions & 0 deletions clang/test/CodeGen/debug-dir-win-pe-magic-sections.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// This test checks if COFF file compiled with
// -fprofile-generate has magic section ".pgi" to indicate so.

// REQUIRES: x86-registered-target

// RUN: %clang --target=x86_64-pc-windows -fprofile-generate %s -c -o %t_x86
// RUN: llvm-objdump -h %t_x86 | FileCheck --check-prefix=CHECK_PGI %s

// CHECK_PGI: {{.*}}.pgi{{.*}}

// This test checks if COFF file contains a magic ".pgu" section to indicate that
// it was compiled using profiling data.

// RUN: llvm-profdata merge -output=%code.profdata %S/Inputs/thinlto_expect1.proftext
// RUN: %clang --target=x86_64-pc-windows -fprofile-use=%code.profdata -c %s -o %t.obj
// RUN: llvm-objdump -h %t.obj | FileCheck --check-prefix=CHECK_PGU %s

// CHECK_PGU: {{.*}}.pgu{{.*}}

int main(void) {

return 0;
}
54 changes: 53 additions & 1 deletion lld/COFF/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ static unsigned char dosProgram[] = {
static_assert(sizeof(dosProgram) % 8 == 0,
"DOSProgram size must be multiple of 8");

static char ltcg[] = "LTCG";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr char ...[]

constexpr/const make this internal linkage, making static unneeded

static char pgi[] = "PGI";
static char pgu[] = "PGU";
static char pgiSectionName[] = ".pgi";
static char pguSectionName[] = ".pgu";

static const int dosStubSize = sizeof(dos_header) + sizeof(dosProgram);
static_assert(dosStubSize % 8 == 0, "DOSStub size must be multiple of 8");

Expand Down Expand Up @@ -179,6 +185,23 @@ class ExtendedDllCharacteristicsChunk : public NonSectionChunk {
uint32_t characteristics = 0;
};

class DebugDirStringChunk : public NonSectionChunk {
public:
DebugDirStringChunk(std::string str) : str(str.begin(), str.end()) {
while (this->str.size() % 4 != 0)
this->str.push_back(0);
}
size_t getSize() const override { return str.size(); }

void writeTo(uint8_t *b) const override {
char *p = reinterpret_cast<char *>(b);
auto strReverse = str;
std::reverse(strReverse.begin(), strReverse.end());
memcpy(p, strReverse.data(), strReverse.size());
}
std::vector<char> str;
};

// PartialSection represents a group of chunks that contribute to an
// OutputSection. Collating a collection of PartialSections of same name and
// characteristics constitutes the OutputSection.
Expand Down Expand Up @@ -1165,6 +1188,22 @@ void Writer::createMiscChunks() {
llvm::TimeTraceScope timeScope("Misc chunks");
Configuration *config = &ctx.config;

auto searchForPgoMagicSection = [this](char sectionName[]) {
for (auto *obj : ctx.objFileInstances) {
for (auto &chunk : obj->getChunks()) {
if (chunk->kind() == Chunk::SectionKind &&
chunk->getSectionName() == sectionName) {
return true;
}
}
}
return false;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this? Why can we not just emit the magic content with COMDAT and let /debug handle the preservation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand you correctly, you suggest creating a section holding the debug dir during COFF file emission, with expectation that the linker will preserve it, right? But then we would have to update the "Debug" field of the of the "Optional Header Data Directories" of Windows PE file to point to the debug directory, which I believe would more or less be the same to the current solution, we would have to iterate over all section of all object files to update the "Debug" entry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, my thinking was that if we have content that is guaranteed to be folded into the debug data directory, the directory will be emitted. As such, the linker will link the directory in the header and emit that. This would avoid the need to iterate all the sections, it would simply force the emission of the debug directory without /debug being passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this content is guaranteed to be folded into the debug data directory. I can create a debug dir entry in the COFF with COMDAT, and this will be folded, but then I still have to adjust pointers in the optional header Debug field to point to debug dir, and to specific entries in the debug dir AddressOfRawData field. This has to be done during the linking phase, because this structures don't exist in the COFF files


bool writePgi = searchForPgoMagicSection(pgiSectionName);
bool writePgu = !writePgi && searchForPgoMagicSection(pguSectionName);
bool writeLTO = ctx.bitcodeFileInstances.size();

for (MergeChunk *p : ctx.mergeChunkInstances) {
if (p) {
p->finalizeContents();
Expand All @@ -1181,7 +1220,7 @@ void Writer::createMiscChunks() {
// Create Debug Information Chunks
debugInfoSec = config->mingw ? buildidSec : rdataSec;
if (config->buildIDHash != BuildIDHash::None || config->debug ||
config->repro || config->cetCompat) {
config->repro || config->cetCompat || writePgi || writePgu || writeLTO) {
debugDirectory =
make<DebugDirectoryChunk>(ctx, debugRecords, config->repro);
debugDirectory->setAlignment(4);
Expand All @@ -1206,6 +1245,19 @@ void Writer::createMiscChunks() {
IMAGE_DLL_CHARACTERISTICS_EX_CET_COMPAT));
}

if (writeLTO) {
debugRecords.emplace_back(COFF::IMAGE_DEBUG_TYPE_POGO,
make<DebugDirStringChunk>(ltcg));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t mean to discourage you @mikolaj-pirog. But if you could come up with a proper structure here for IMAGE_DEBUG_TYPE_POGO, I think the PR would be acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t mean to discourage you @mikolaj-pirog. But if you could come up with a proper structure here for IMAGE_DEBUG_TYPE_POGO, I think the PR would be acceptable.

No worry, you didn't discourage me, I appreciate each piece of feedback :) Just to be clear, would this patch be accepted if I manage to make lld emit the appropriate structure (like MSVC does) for the PGO/PGU/LTCG?

}

if (writePgi) {
debugRecords.emplace_back(COFF::IMAGE_DEBUG_TYPE_POGO,
make<DebugDirStringChunk>(pgi));
} else if (writePgu) {
debugRecords.emplace_back(COFF::IMAGE_DEBUG_TYPE_POGO,
make<DebugDirStringChunk>(pgu));
}

// Align and add each chunk referenced by the debug data directory.
for (std::pair<COFF::DebugType, Chunk *> r : debugRecords) {
r.second->setAlignment(4);
Expand Down
15 changes: 15 additions & 0 deletions lld/test/COFF/debug_dir_magic_strings_from_section_pgi.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// This test checks if lld puts magic string "PGI" when an object files contains
// .pgi section.

// REQUIRES: x86

// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-windows %s -o %t.main_x86.obj

// RUN: lld-link -out:%t_x86.exe %t.main_x86.obj -entry:entry -subsystem:console -debug:symtab
// RUN: llvm-readobj --coff-debug-directory %t_x86.exe | FileCheck --check-prefix=CHECK_PGI %s
// CHECK_PGI: {{.*}}IGP{{.*}}

#--- main.s
.section .pgi
.global entry
entry:
15 changes: 15 additions & 0 deletions lld/test/COFF/debug_dir_magic_strings_from_section_pgu.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// This test checks if lld puts magic string "PGU" when an object files contains
// .pgu section.

// REQUIRES: x86

// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-windows %s -o %t.main_x86.obj

// RUN: lld-link -out:%t_x86.exe %t.main_x86.obj -entry:entry -subsystem:console -debug:symtab
// RUN: llvm-readobj --coff-debug-directory %t_x86.exe | FileCheck --check-prefix=CHECK_PGU %s
// CHECK_PGU: {{.*}}UGP{{.*}}

#--- main.s
.section .pgu
.global entry
entry:
12 changes: 12 additions & 0 deletions lld/test/COFF/debug_dir_magic_strings_lto.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
; REQUIRES: x86
; RUN: llvm-as -o %main.obj %s
; RUN: lld-link /out:%main.exe /entry:main /subsystem:console %main.obj
; RUN: llvm-readobj --coff-debug-directory %main.exe
; CHECK: {{.*}}GCTL{{.*}}

target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows"

define i32 @main() {
ret i32 0
}
2 changes: 2 additions & 0 deletions llvm/include/llvm/MC/MCTargetOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ class MCTargetOptions {
// Whether or not to use full register names on PowerPC.
bool PPCUseFullRegisterNames : 1;

bool PgoInstrumentation = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target options like this don't play well with (thin)LTO , because they don't carry over naturally from the frontend compilation step to the backend compilation step, which LTO separates. Is there an existing global named metadata flag you can look for instead to control this debug info setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I am aware, there isn't any global metadata flag I could fetch from within MC. Could you elaborate a bit more when the current solution would cause problems? I am not that familiar with LTO inner workings

bool PgoUse = false;
MCTargetOptions();

/// getABIName - If this returns a non-empty string this represents the
Expand Down
13 changes: 13 additions & 0 deletions llvm/lib/MC/WinCOFFObjectWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "llvm/MC/MCSectionCOFF.h"
#include "llvm/MC/MCSymbol.h"
#include "llvm/MC/MCSymbolCOFF.h"
#include "llvm/MC/MCTargetOptions.h"
#include "llvm/MC/MCValue.h"
#include "llvm/MC/MCWinCOFFObjectWriter.h"
#include "llvm/MC/StringTableBuilder.h"
Expand Down Expand Up @@ -981,6 +982,18 @@ static std::time_t getTime() {
uint64_t WinCOFFWriter::writeObject(MCAssembler &Asm) {
uint64_t StartOffset = W.OS.tell();

const auto *Options = Asm.getContext().getTargetOptions();

if (Mode != DwoOnly && Options && Options->PgoInstrumentation) {
auto *Section = Asm.getContext().getCOFFSection(".pgi", 0);
defineSection(Asm, *Section);
}

if (Mode != DwoOnly && Options && Options->PgoUse) {
auto *Section = Asm.getContext().getCOFFSection(".pgu", 0);
defineSection(Asm, *Section);
}

if (Sections.size() > INT32_MAX)
report_fatal_error(
"PE COFF object files can't have more than 2147483647 sections");
Expand Down
Loading