Skip to content
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

Clang 16 fixes #472

Closed

Conversation

JamesWrigley
Copy link
Member

@JamesWrigley JamesWrigley commented Mar 4, 2024

This is a speculative attempt to fix Clang 16 compatibility. My understanding of the problem (helped by the folks on the LLVM Discourse) is that Clang 16 represents way more types as ElaboratedType's than before: llvm/llvm-project@15f3cd6

This PR fixes that by looking at the named type instead in most places. It's probably worth testing this on some bigger projects because I'm not fully confident that it's doing the right thing (it's also a little hideous to normalize the types everywhere ☹️ ).

I haven't figured out how to do it yet, but I think what we really want to do is determine whether an ElaboratedType has any elaboration and then desugar it if so. That seems like the 'right' thing to do compared to blindly desugaring for everything but enums.

EDIT: two odd things:

  • The tests stdout is munged, maybe it's a 1.11 thing?
  • Now only the documentation tests are failing.

ref: https://reviews.llvm.org/D112374
Co-Authored-By: James Wrigley <JamesWrigley@users.noreply.github.com>
@Gnimuc
Copy link
Member

Gnimuc commented Mar 6, 2024

I pushed a similar fix in bcab1c7.

The following chunk of code basically reads:

  • if the type is directly or indirectly reference a tag type, and this tag type is in the dag.tags, then add it as a dependency
  • if the type is not directly or indirectly reference any tag type but this leaf type is in the dag.ids, then add it as a dependency

hasref = has_elaborated_reference(ty)
if hasref && haskey(dag.tags, leaf_ty.sym)
push!(node.adj, dag.tags[leaf_ty.sym])
elseif !hasref && haskey(dag.ids, leaf_ty.sym)
push!(node.adj, dag.ids[leaf_ty.sym])

Clang.jl/src/type.jl

Lines 270 to 287 in e1a96ae

"""
has_elaborated_reference(ty::CLType) -> Bool
has_elaborated_reference(ty::CXType) -> Bool
Return true if the type is an elaborated type or the type indirectly refers to an
elaborated type.
"""
function has_elaborated_reference(ty::CXType)
if kind(ty) == CXType_Pointer
ptreety = getPointeeType(ty)
return has_elaborated_reference(ptreety)
elseif kind(ty) == CXType_ConstantArray || kind(ty) == CXType_IncompleteArray
elty = getElementType(ty)
return has_elaborated_reference(elty)
else
return is_elaborated(ty)
end
end
has_elaborated_reference(ty::CLType) = has_elaborated_reference(ty.type)

so, the old has_elaborated_reference should be has_elaborated_tag_reference. Note we need to recursively check the leaf referenced type is a tag type.

@Gnimuc
Copy link
Member

Gnimuc commented Mar 6, 2024

I suspect the documentation failure is related to the following contents. The range of the source code and comments are somehow miss-calculated for "elaborated" cursors.

  1. This patch could expose a bug in how you get the source range of some
    TypeLoc. For some reason, a lot of code is using getLocalSourceRange(),
    which only looks at the given TypeLoc node. This patch introduces a new,
    and more common TypeLoc node which contains no source locations on itself.
    This is not an inovation here, and some other, more rare TypeLoc nodes could
    also have this property, but if you use getLocalSourceRange on them, it's not
    going to return any valid locations, because it doesn't have any. The right fix
    here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive
    into the inner TypeLoc to get the source range if it doesn't find it on the
    top level one. You can use getLocalSourceRange if you are really into
    micro-optimizations and you have some outside knowledge that the TypeLocs you are
    dealing with will always include some source location.

@JamesWrigley
Copy link
Member Author

Neat, thanks ❤️ I think I figured out the documentation issue too, looks like we're hitting this bug: JuliaLang/julia#52986
Fixed in 08b1ad2. I think both this and #465 can be merged now 🤞 (I removed my hacky fix)

@Gnimuc Gnimuc deleted the branch JuliaInterop:clang16 March 6, 2024 13:45
@Gnimuc Gnimuc closed this Mar 6, 2024
@Gnimuc
Copy link
Member

Gnimuc commented Mar 6, 2024

Sorry. 😓 I did a rebase merge. Could you rebase this onto the master branch?

@JamesWrigley
Copy link
Member Author

No worries, will do :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants