Page MenuHomePhabricator

[lld/mac] Make symbol table order deterministic
ClosedPublic

Authored by thakis on Jun 28 2021, 5:15 PM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rGaed0a08c69cf: [lld/mac] Make symbol table order deterministic
Summary

SymtabSection::emitStabs() writes the symbol table in the order
of externalSymbols, which has the order of symtab->getSymbols(),
which is just the order symbols are added to the symbol table.

In practice, symbols in the symbol files of input .o files are
sorted, but since that's not guaranteed we sort them in
ObjFile::parseSymbols(). To make sure several symbols with the same
address keep the order they're in the input file, we have to use
stable_sort().

In practice, std::sort() on already-sorted inputs won't change the order
of just adjacent elements, and while in theory std::sort() could use a
random pivot, in practice the code should be deterministic as it was
previously too.

But now lld/test/MachO/stabs.s passes with LLVM_ENABLE_EXPENSIVE_CHECKS=ON
(the last test that was failing with that set).

Fixes a regression from D99972.

While here, remove an empty section in stabs.s and move
.subsections_via_symbols to the end where it usually is (this part no
behavior change).

Diff Detail

Event Timeline

thakis created this revision.Jun 28 2021, 5:15 PM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgrang. · View Herald Transcript
thakis requested review of this revision.Jun 28 2021, 5:15 PM
int3 accepted this revision.Jun 28 2021, 8:01 PM

Thanks! Would be nice to have LLVM_ENABLE_EXPENSIVE_CHECKS=ON running as part of some buildbot...

This revision is now accepted and ready to land.Jun 28 2021, 8:01 PM

Thanks!

Would be nice to have LLVM_ENABLE_EXPENSIVE_CHECKS=ON running as part of some buildbot...

Agreed. Looks like there are bots with expensive checks enabled here:
https://lab.llvm.org/buildbot/#/builders/16
https://lab.llvm.org/buildbot/#/builders/104

But they only -DLLVM_ENABLE_PROJECTS=llvm, not lld. Maybe we can figure out who owns them and ask them to add lld? …Hm, one of them was added in D69359. @vvereschaka, could we run lld tests on that bot too? lld tests run pretty quickly.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 6:30 AM

Hi @thakis,

@vvereschaka, could we run lld tests on that bot too? lld tests run pretty quickly.

ok, it shouldn't be a problem. I'll take a look and update the builder configurations accordingly.

Hi @thakis,

@vvereschaka, could we run lld tests on that bot too? lld tests run pretty quickly.

ok, it shouldn't be a problem. I'll take a look and update the builder configurations accordingly.

Thanks! :)

I did some research and I found that lld project does not contain any expensive checks. There is nothing to run on the expensive check builders.
But there is few builders that run the lld tests including MachO/stabs.s. As example lld-x86_64-ubuntu-fast - https://lab.llvm.org/buildbot/#/builders/58

int3 added a comment.Jun 30 2021, 3:38 PM

I did some research and I found that lld project does not contain any expensive checks.

Maybe I'm misunderstanding, but we did have tests that were failing when configured with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON. We don't have LLD code that ifdef's on EXPENSIVE_CHECKS, but we use a bunch of LLVM data structures which do

Now I got it, thank you. I have prepared proper changes here https://reviews.llvm.org/D105369