This is an archive of the discontinued LLVM Phabricator instance.

[llvm-nm] Print EC symbol map.
ClosedPublic

Authored by jacek on Mar 21 2023, 8:54 AM.

Details

Summary

This is useful for examining ARM64EC static libraries and allows better llvm-lib testing. Changes to Archive class will also be useful for LLD to support ARM64EC, where it will need to use one map or the other, depending on linking target (or both, in case of ARM64X, but separately as they are in different namespaces).

Diff Detail

Event Timeline

jacek created this revision.Mar 21 2023, 8:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
jacek requested review of this revision.Mar 21 2023, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2023, 8:54 AM

This code seems like it isn't being careful to ensure calls to read16le/read32le/etc. don't read past the end of a buffer. Maybe it makes sense to add some sort of bounds-checked read helper? Maybe could leave it for a followup, though, if it's confusing to mix it in with this patch.

llvm/lib/Object/Archive.cpp
951

This could use a comment explaining what it's doing... I assume it's looking for the EC symbol table, but it's not obvious how the member ordering here works.

1007

The math with the symbol indexes all over is confusing... maybe consider adding a helper to check "does this symbol index refer to an EC symbol"?

1116

What happens if find() fails?

jhenderson added a comment.EditedMar 22 2023, 1:11 AM

What are "ECSymbols"? Is there a spec I can read for them, so that I can review this change?

What are "ECSymbols"?

Those are "emulation compatible" symbols used for ARM64EC support:
https://learn.microsoft.com/en-us/cpp/build/arm64ec-windows-abi-conventions?view=msvc-170
ARM64EC allows linking a special ARM64 object files together with x86_64 object files. It's also possible to build ARM64X binaries containing both pure ARM64 and ARM64EC code, in which case both have separated namespaces and, depending on process loading the module, only one variant is used in runtime.

For static libraries, it's allowed to have both ARM64 and ARM64EC in a single library. Since they need separated namespaces, they can't share symbol map. ARM64EC uses <ECSYMBOLS> instead of the usual map.

Is there a spec I can read for them, so that I can review this change?

No, there is no scpec that I'm aware of. I figured the format by inspecting libraries produced by MSVC lib.exe. I will add more comments to the code, but the format is very similar to usual COFF symbol map:

  • 32-bit LE number of symbols
  • followed by an array of 16-bit member index for each symbol. This is an index in offset table from the regular COFF symbol table (it there is no separated offset table)
  • followed by null-terminated strings names for each symbol in the same order as indexes above

See also writeECSymbols from https://reviews.llvm.org/D143541

jacek updated this revision to Diff 507796.Mar 23 2023, 10:27 AM

Thanks for review. The new version adds more comments, validates EC symbol map and introduces isECSymbol helper.

This code seems like it isn't being careful to ensure calls to read16le/read32le/etc. don't read past the end of a buffer. Maybe it makes sense to add some sort of bounds-checked read helper? Maybe could leave it for a followup, though, if it's confusing to mix it in with this patch.

I originally followed how current code does, which just assumes that symbol table is valid. The problem with bounds-checked helper is that we don't really have a good way of propagating errors from things like iterators. In the new version, I added an early validation of EC symbols map so that if it's present, we know that all those reads are safe. I wasn't sure what to do when the map is invalid. We could treat it as an error in Archive constructor, but I decided to just ignore the map instead assuming that the archive may still be useful.

efriedma added inline comments.Mar 23 2023, 10:54 AM
llvm/lib/Object/Archive.cpp
986

StringRef::find() returns npos (i.e. -1) on failure, so I don't think this does what you want.

jacek updated this revision to Diff 507828.Mar 23 2023, 11:14 AM

I misinterpreted docs, sorry. Fixed in the new version.

This revision is now accepted and ready to land.Mar 23 2023, 11:41 AM

I wasn't sure what to do when the map is invalid. We could treat it as an error in Archive constructor, but I decided to just ignore the map instead assuming that the archive may still be useful.

Actually, hmm... maybe we should reject in this case. I mean, the archive might be "useful" in the sense that it contains interesting object files someone might want to use, but what's the linker supposed to do if symbol map is malformed? If we eat the error, it'll just assume there just aren't any symbols, so it'll produce weird link errors or an invalid executable.

jacek added a comment.Mar 23 2023, 1:57 PM

I wasn't sure what to do when the map is invalid. We could treat it as an error in Archive constructor, but I decided to just ignore the map instead assuming that the archive may still be useful.

Actually, hmm... maybe we should reject in this case. I mean, the archive might be "useful" in the sense that it contains interesting object files someone might want to use, but what's the linker supposed to do if symbol map is malformed? If we eat the error, it'll just assume there just aren't any symbols, so it'll produce weird link errors or an invalid executable.

My thinking was that things like llvm-lib -extract or merging with llvm-lib (which would generate a new map) could still work. Linker could do whole archive import and build pure ARM64 binaries (although I guess we should also have validation for regular map, which has similar consequences). I think we could move validation to ec_symbols() so that callers that need it may handle error.

jacek updated this revision to Diff 507875.Mar 23 2023, 2:00 PM

New version with fallible ec_symbols(), fixed member index check (it's 1-based) and a test for malformed EC symbols.

efriedma accepted this revision.Mar 23 2023, 2:35 PM

There's only one tool in existence that produces archives with an ecsymbols section; with your patch to llvm, there will be two. Corrupted sections aren't actually a thing anyone is likely to run into, outside of random data corruption. Certainly not as part of a normal build process.

But I guess delaying the error check until we actually try to query the table doesn't really do any harm. Sure, this is fine.

Looks to me like you should have test cases for your EC Map validation.

llvm/lib/Object/Archive.cpp
952–955
1193

The LLVM coding standards state you should give additional context in error messages that would help diagnose the issue better (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages) In this case, you could say "invalid EC symbol table size (<ECSymTable.size()>)".

1197

As above, more context could be included, e.g. "invalid EC symbols size. Size was <ECSymbolTable.size()>, but expected <sizeof(uint32_t) + Count * sizeof(uint16_t))>" (obviously expanding the bits in "<>").

1201

If you moved this variable definition before the previous error check, you'd be able to reuse it in that error check, rather than duplicating the calculation.

1206

As above, include the index and member count in the message for additional context.

1209

Perhaps: "malformed EC symbol names: not null-terminated" to clarify to the user why it appears malformed? Also, note "symbols" -> "symbol" regardless of the additional context.

jacek updated this revision to Diff 508177.Mar 24 2023, 11:38 AM

Thanks! The new version adds more context to error messages and more validation tests.

jhenderson added inline comments.Mar 27 2023, 12:56 AM
llvm/lib/Object/Archive.cpp
952–955

Looks like this comment was ignored?

jacek added inline comments.Mar 27 2023, 4:18 AM
llvm/lib/Object/Archive.cpp
952–955

Sorry about that (I looked mostly at mail form of the comment, in which it looked like a context). I will prepare a new version.

jacek updated this revision to Diff 508569.Mar 27 2023, 4:24 AM

Fixed comment.

jhenderson accepted this revision.Mar 27 2023, 4:41 AM

LGTM. FWIW, you didn't need to post a review for this sort of minor fix.

This revision was landed with ongoing or failed builds.Apr 21 2023, 5:46 AM
Closed by commit rG85a2c50ec497: [llvm-nm] Print EC symbol map. (authored by jacek, committed by mstorsjo). · Explain Why
This revision was automatically updated to reflect the committed changes.

Hello and good afternoon from the UK,

I believe this is 2 of 3 patches that may have caused the following build bot to begin failing, are you able to take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/20269

Warm regards,
Tom W

It's also causing an error on the MemorySanitizer build (https://lab.llvm.org/buildbot/#/builders/74/builds/18749/steps/14/logs/stdio), which shows the uninitialized memory usage.

jacek added a comment.Apr 21 2023, 1:10 PM

I think I can see what's happening. Child::StartOfFile is unintialized in this case, but it should be harmless because it's not actually used (I think the same would happen without the patch when listing an empty archive). I will submit a fix after a bit more testing.