Skip to content

Revert "Documentation: Remove warning that nonstandard primitive type sizes may reveal LLVM bugs" #58439

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LilithHafner
Copy link
Member

Reverts #58262

See #58434, for an example bug.

@LilithHafner LilithHafner added the docs This change adds or pertains to documentation label May 16, 2025
Per #58434 (comment), seems like this might not be an LLVM bug.
@LilithHafner LilithHafner added revert This reverts a previously merged PR. types and dispatch Types, subtyping and method dispatch labels May 16, 2025
@Seelengrab
Copy link
Contributor

Seelengrab commented May 16, 2025

#58434 is about a multiple of 8 though, how does that issue show issues with non-multiples of 8? The bug is also unrelated to the exact size, since it's about padding when placed within a struct.

@PatrickHaecker
Copy link
Contributor

From reading through the analysis done in #58434 and the PR in #58435, I don't think, that fully reverting #58262 is the correct thing to do.

This is the summary I got from these discussions (with some interpretation from me included):

  • A Julia bug was identified, which manifests when using a global Tuple/struct of a non-power-of-2 primitive type and using the === comparison (but not the == comparison)
  • The bug is inside Julia, as we didn't indicate that a data structures needs and uses padding when using a non-power-of-2 primitive type which then lead to a sometimes incorrect comparison with === (depending on the values of the padded bits which were accidentally compared, but not set).
  • In my understanding not using padding would also be a violation of the C API that we want to follow by default.
  • The bug should be fixed with Fix layout flags for types that have oddly sized primitive type fields #58435
  • This fix should be backported to 1.10, 1.11 and 1.12
  • It's plausible, that the bug was not identified by, e.g. BitIntegers.jl's test cases, because they do neither cover globals nor composite types nor tuples and comparison with these types is probably more often done with == than with ===.
  • There is still no indication that LLVM has bugs in this area.

The fact that LLVM had bugs in the past in an area where we now also have/had a bug, does not seem to logically conclude that LLVM therefore must be buggy.

We might consider adding a generic warning, that "using nonstandard primitive type sizes may reveal bugs" (note the absence of LLVM in that phrase), but let's also stick to the data here. We now had one bug in this area in which time frame? Years?
If we'd put a warning into the documentation for some years whenever we had a single bug in an area, we'd need to document a lot.
I'd prefer to only document it if there is an accumulation of bugs or if they cannot be fixed in a limited amount of time.

@PatrickHaecker
Copy link
Contributor

#58434 is about a multiple of 8 though, how does that issue show issues with non-multiples of 8? The bug is also unrelated to the exact size, since it's about padding when placed within a struct.

I guess this is a rhetorical question, but you made me curios:
We can't test this, as currently no such primitive types can be created. In the absence of additional bugs, I'd expect the very same behavior: Without the fix from #58435 we should see the same problems in structs/tuples. With the fix it should work.

And indeed, when you deactivate the last part of this check with the fix on top of Julia master, this works correctly (repeatedly):

julia> module Foo
                  primitive type UInt33 33 end
                  a = reinterpret(UInt33, ntuple(_ -> 0x0, 5))
                  res = (a,) === (a,)
                  f(x) = (x,) === (x,)
                  res2 = f(a)
                  g() = (a,) === (a,)
                  res3 = g()
                  function h()
                      x = reinterpret(UInt33, ntuple(_ -> 0x0, 5))
                      return (x,) === (x,)
                  end
                  res4 = h()
              end; using .Foo; (Foo.res, Foo.res2, Foo.res3, Foo.res4)
(true, true, true, true)

However, I am undecided on whether the reinterpret is a bug or a feature.
On the one hand it seems to be the only way to create such an object and given how you would map this type to real hardware constraints, it's probably (haven't checked) more or less guaranteed by LLVM that this should work.
On the other hand, this looks horribly wrong.

But before we decided on how to construct these objects, all methods besides == and === seem kind of mood.

@Seelengrab
Copy link
Contributor

On the one hand it seems to be the only way to create such an object

You can do Core.zext_int(UInt33, 0x0), which would be the correct way of doing that. The reinterpret there is technically not ok, since you're moving data into padding (in the best case, in the worst case you're just dropping allocated memory on the floor) and roundtripping back to that tuple type would read that padding (or uninitialized memory), which would be UB.

On the other hand, this looks horribly wrong.

Nah, having non-multiple-of-8-bits primitive types is perfectly fine & valid. For example, zig has them:

Zig supports arbitrary bit-width integers, referenced by using an identifier of i or u followed by digits. For example, the identifier i7 refers to a signed 7-bit integer. The maximum allowed bit-width of an integer type is 65535.

Zig used to use/has used LLVM as a backend, making use of the same functionality Julia uses for primitive types. I think that's a good argument for that feature of LLVM being pretty stable & correct by now.

@PatrickHaecker
Copy link
Contributor

On the other hand, this looks horribly wrong.

Nah, having non-multiple-of-8-bits primitive types is perfectly fine & valid.

Sorry, this was ambiguous. I was referring to the point that interpreting a 40 bits type as a 33 bits type look horribly wrong (which you confirmed). I agree, that non-multiple-of-8-bits primitive types are fine in general (when Julia supports them properly).

You can do Core.zext_int(UInt33, 0x0), which would be the correct way of doing that.

Thanks for this tip. However, it does not work in all cases:

julia> Core.zext_int(UInt7, 0x0)
ERROR: ZExt: output bitsize must be > input bitsize

@LilithHafner LilithHafner requested a review from oscardssmith May 17, 2025 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation revert This reverts a previously merged PR. types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants