Switch to single precompiled header in macro fallback #2823
Conversation
I tested, seems like it still doesn't generate the IRQ defines when I have other headers included |
|
@SeleDreams Can you provide me with a minimal reproducer to reproduce your problem with the headers you're using? I'm no longer able to reproduce the problem with this PR in the setup that originally helped me confirm that there was a bug. |
There was a problem hiding this comment.
This looks reasonable but I guess we should sort out why it's still not working for @SeleDreams? Or do you want this merged now?
A test for this would also be amazing, but if it's too hard to set up lmk.
|
I'll try to do a reproduction case as soon as possible.
Right now with my great luck I have an X server issue on the linux machine
I work on so I can't do it right now
Le lun. 29 avr. 2024 à 20:02, John Baublitz ***@***.***> a
écrit :
… @SeleDreams <https://github.com/SeleDreams> Can you provide me with a
minimal reproducer to reproduce your problem with the headers you're using?
I'm no longer able to reproduce the problem with this PR in the setup that
originally helped me confirm that there was a bug.
—
Reply to this email directly, view it on GitHub
<#2823 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD4UF4PB56GDOHDE2TOYJHDY72DNHAVCNFSM6AAAAABG6PCDBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGM2DANJRGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I would like to figure out what's going on with @SeleDreams before we merge this as they reported the problem originally so I'd like to make sure it works in their use case too. If it's a setup problem, I'll help debug and if there are two bugs, I'll fix the second one. :)
How would you generally suggest I go about testing multiple headers? Is there infrastructure for that? |
Okay sounds good! Just let me know when you can provide the reproducer, and I'll take a look. Basically I just want to rule out a setup problem because since bindgen doesn't have a lot of error reporting infrastructure yet (I think it's planned), setup problems will look exactly the same as a bug: bindings for the constants are just not generated. If it's a bug, I'll fix it. If not, I'll try to point you to exactly what's going on. |
|
@jbaublitz So, I was able to make a reproduction project, however, for some weird reason, it seems to delete the wrapper.h as well everytime I build the project which in itself could be another bug |
|
@SeleDreams Thanks so much for the report and taking the time to craft a reproducer! It appears there were two bugs. One was that Clang only supports one precompiled header at a time which I resolved, but the other is that The resolution I've come to is forwarding all |
jbaublitz
force-pushed
the
bindgen-fallback-fix
branch
3 times, most recently
from
4f6af27
to
23237c5
Compare
|
I've added a test that tests both multiple headers and also the |
jbaublitz
force-pushed
the
bindgen-fallback-fix
branch
from
23237c5
to
6e1c9ad
Compare
There was a problem hiding this comment.
Is there any good reason not to forward all flags? Think of a macro that desugars to something like sizeof(int), if you don't pass the right flags (like --target and so) you might end up with the wrong constant value, right? Same with sizeof(enum foo) if you don't pass -fshort-enums or what not... Probably countless examples too.
The reason I didn't forward all flags is that generally there's a header provided at the end of the clang args in my tests which causes the precompilation to fail. If you'd prefer, I can just remove the last argument instead of exclusively passing the include directives. |
|
Yes. Excluding what we don't need seems like a better approach imho |
jbaublitz
force-pushed
the
bindgen-fallback-fix
branch
from
6e1c9ad
to
ff3793a
Compare
|
@emilio I've taken the approach of filtering out any argument that matches the elements in @SeleDreams Once the code review is done, do you mind testing your actual setup and confirming the problem is resolved? |
It appears that Clang only supports a single precompiled header at a time. Because the macro fallback depends on the ability to provide multiple precompiled headers at once, this commit changes the code to include all provided headers into a single header to precompile and then pass to the TranslationUnit. This should resolve the issue where the macro fallback would not function as intended when multiple headers were provided as input. This commit also resolves an issue where clang args passed to the builder were not forwarded to the precompilation translation unit, resulting in headers not in standard system directories not being found.
jbaublitz
force-pushed
the
bindgen-fallback-fix
branch
from
ff3793a
to
0178a46
Compare
|
@SeleDreams I'm assuming this resolved your issue? If not, please let me know. |
|
that seems to have worked |
Fix for issue reported here.
@SeleDreams Could you also confirm that this resolves your problem?