I verified that the failing tests don't break locally.

The test failures in libomp don't look related to me.

This should be ready for review now.

So this was a fun one. The previous version of this patch accidentally caused a
pre-existing bug to surface and break Chromium. In certain circumstances
including the Chromium test case, available_externally symbols would cause
invalid lookups in the SymSize table. Before this patch, the lookups were
done with the [] operator, which would succeed but cause an unspecified value
to be emitted in the .debug_aranges table. But this patch moves those lookups
to be done by dereferencing the result of the .find() method, which causes
asserts if the key is not found.

Minimal reproduction:

Updated the diff to address comments.

Test failures look unrelated?

Abandoned in favor of D126835.

I see. I don't have commit access unfortunately.

Sorry for the delay. I was looking at this on Friday but didn't get around to finishing it. Feel free to revert in the meantime.

Closing in favor of D126257.

Oh, what I mean is that our choices in the case in which zero-sized global symbols are forbidden at the IR level would be (1) don't emit source-level zero-sized symbols into LLVM IR (or don't emit DWARF metadata for them), or (2) round all zero-sized symbols up to one byte. If we pick (1), I don't think they would show up in the debug info, which could be confusing for programmers. If we pick (2), then certain abstractions which are zero-cost today in Rust become non-zero-cost.

Wouldn't that mean that those zero-sized data symbols won't show up in the debugger anymore? That might be confusing for Rust users, who do use zero-sized globals reasonably commonly (to attach methods to).

How's this?

Emit a constant 1 instead of a more complicated MCExpr when emitting symbols of length 1.

@dblaikie Here's a new version of the patch that takes the alternate approach you suggested of rounding lengths up to 1 byte. Feel free to take either diff and I'll close the other.

I like the idea of moving in the direction of forbidding zero-sized functions (pretty sure Rust never emits these) but supporting zero-sized globals.

Added a test.

That being said, forbidding global zero-sized symbols might work for us, because I think those are pretty rare. They do appear in practice sometimes, but not commonly.

Well, named zero-sized values are a first-class feature of Rust. You can write code like this:

Looks fine to me. Note that to trigger on Debug we also need the frontend changes at https://github.com/rust-lang/rust/pull/88832.

Addressed review comments.

Addressed comments. Reverted the EAX save refactoring.

Addressed review comments.

Addressed comments.

In D34387#785802, @whitequark wrote:

@pcwalton No, both the spill and the "magic" number are necessary. See https://reviews.llvm.org/D9858. ebx is the size of the frame, and 0x1000 is the probe interval to be performed (in other words, page size).

Addressed comments.

Addressed review comments.

Added more context, added missing tests, removed whitespace-only changes.

Split this out correctly, added more context, and added the missing test.