-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
f903e7e
9dfc603
aff0c90
5cb0237
0e3749f
c98e758
8f2b91a
46e70ab
a5bd283
d999486
adb3b39
5c0477c
1f2a088
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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"); | ||
|
||
|
@@ -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. | ||
|
@@ -1165,6 +1188,22 @@ void Writer::createMiscChunks() { | |
llvm::TimeTraceScope timeScope("Misc chunks"); | ||
Configuration *config = &ctx.config; | ||
|
||
auto searchForPgoMagicSection = [this](char sectionName[]) { | ||
mikolaj-pirog marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (auto *obj : ctx.objFileInstances) { | ||
for (auto &chunk : obj->getChunks()) { | ||
if (chunk->kind() == Chunk::SectionKind && | ||
chunk->getSectionName() == sectionName) { | ||
return true; | ||
} | ||
} | ||
} | ||
return false; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
bool writePgi = searchForPgoMagicSection(pgiSectionName); | ||
bool writePgu = !writePgi && searchForPgoMagicSection(pguSectionName); | ||
bool writeLTO = ctx.bitcodeFileInstances.size(); | ||
|
||
for (MergeChunk *p : ctx.mergeChunkInstances) { | ||
if (p) { | ||
p->finalizeContents(); | ||
|
@@ -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); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
|
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: |
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: |
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,8 @@ class MCTargetOptions { | |
// Whether or not to use full register names on PowerPC. | ||
bool PPCUseFullRegisterNames : 1; | ||
|
||
bool PgoInstrumentation = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Uh oh!
There was an error while loading. Please reload this page.