vsapsai wrote:
I'll be busy during WWDC week too, so no real changes are expected in the next
2 weeks.
https://github.com/llvm/llvm-project/pull/138227
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/list
jansvoboda11 wrote:
> What's worse, it really depends on whether any `FileID` used by that file was
> ever deserialized
That reminds me of the issue @alexfh and I discussed here:
* [[clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead
of SourceLocation](https://reviews.
ilya-biryukov wrote:
> So far we are still aiming for a more principled solution. For example, if
> support of a header in multiple modules is a supported feature, then some
> kind of a spec with a more comprehensive testing would be appropriate. It is
> up to you but personally I feel like th
vsapsai wrote:
> Thanks a lot, I am sorry for misreading or misunderstanding your comments
> too. I hope we can put this behind ourselves.
Appreciate the understanding.
> [skip the technical description]
Thanks for the explanation, it gives a lot of food for thought. Still building
a mental
ilya-biryukov wrote:
> I'm sorry that my replies came across as hostile and snarky, I didn't mean it
> that way. Though I'll admit I'm not thrilled about your current approach,
> that's what I was communicating. But that's what you have right now and there
> is no easy way to change it fast.
vsapsai wrote:
What about the approach to provide all_textual.pcm to use.cpp and for clang to
find foo.h in all_textual.pcm? It doesn't work right now but I'm not sure that
it shouldn't (there can be good reasons why it shouldn't work, tbh).
https://github.com/llvm/llvm-project/pull/138227
___
vsapsai wrote:
I think I understand the logic behind your approach.
* Access between headers is enforced through decl-use, so you cannot access
headers from a target that's not your direct dependency.
* But some headers shouldn't be accessed outside of a target [regardless of the
dependencies],
AaronBallman wrote:
We reviewed this thread today at the Clang Area Team meeting and it seems like
y'all have perhaps found a solution everyone is happy with. However, if you'd
like a meeting or if there's some other way we can support you, we're
definitely happy to help!
> Please let me know
ilya-biryukov wrote:
Many thanks for engaging in this discussion and sorry for the delays in my
responses too.
https://github.com/llvm/llvm-project/pull/138227
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mail
ilya-biryukov wrote:
> In general, though, I share the same concern as @vsapsai about leaving this
> patch reverted for too long.
I have a concrete proposal to move forward, #141792. I have checked that the
failure we've seen in our infrastructure goes away with both changes applied.
Given t
cyndyishida wrote:
Firstly, thank you for your responsiveness @ilya-biryukov, and for helping us
understand how this bug fix impacts Google's build.
For all individuals involved in this discussion, I would like to remind that
our [code of conduct](https://llvm.org/docs/CodeOfConduct.html) doe
ilya-biryukov wrote:
> Can you clarify, are you saying this pattern of having a header in two
> different modules has to keep working indefinitely, or are you willing to
> migrate off of it?
> I don't understand what the reason is for the header to be in two different
> modules in the first pl
vsapsai wrote:
Do you include proto headers directly or through the wrapper headers?
Are there any important macros in proto headers?
Do you need to abuse macros to get something out of proto headers? Kinda like
`#define private public` but more realistic.
My initial reaction is to offer a so
cyndyishida wrote:
> And, in the end, the folks debugging this (Ilya & co) are busy working on
> clang header module reproduction and reduction tools:
> https://github.com/emaxx-google/cvise/branches @emaxx-google
Interestingly enough, @vsapsai is also working on reproducers for explicitly
bu
rnk wrote:
(This is mostly written wearing my "Googler" hat, not the clang area team lead
hat)
There's been a lot of discussion of what the exact use case is. I think it's
helpful to share the context that Google essentially uses Clang header modules
to wrap generated proto headers, and this
vsapsai wrote:
> Back to the original issue.
Do you control the module map that specifies a header as private? Does it need
to stay private?
Personally, to me it looks like a bug that a private header is getting picked
up [somewhat accidentally]. I'm not planning to fix this bug, just that re
vsapsai wrote:
> It is concerning to me that we as a company are given **a week** to adjust to
> the change and there is no discussion either about alternative approaches or
> the scale of the changes needed to support this. We do rely on all kinds of
> weird behaviors and side effects in Clan
benlangmuir wrote:
> I don't think we can ever get rid of that pattern and we will keep relying on
> Clang supporting this.
> We would need to example I shared above to keep working.
> I think the change we want is to always prioritize modular over textual,
> before we look at non-private vs p
benlangmuir wrote:
> I don't think we can ever get rid of that pattern and we will keep relying on
> Clang supporting this.
> We would need to example I shared above to keep working.
I don't have a strong opinion on the best path forward here, but I'm not clear
if you're actually willing to
ilya-biryukov wrote:
Back to the original issue.
> I'm willing to help figure out how to achieve the desired result in a
> different way. But for that need to know what is the desired result.
We would need to example I shared above to keep working. We rely on an
optimization that picks a modu
ilya-biryukov wrote:
> Ok, I'm going to revert the change to help you out. But I'm going to re-land
> it in a week or when you are ready, whichever comes first.
>
> There was no indication there is anything wrong with the change or if the
> issue is wide-spread. And if a single company relies o
vsapsai wrote:
Ok, I'm going to revert the change to help you out. But I'm going to re-land it
in a week or when you are ready, whichever comes first.
There was no indication there is anything wrong with the change or if the issue
is wide-spread. And if a single company relies on an existing s
ilya-biryukov wrote:
> And I'm not sure such a use case is worth supporting.
> But I understand that is only my own interpretation which can be incorrect.
> And I want to believe you have a better use case that doesn't rely on
> accessing private headers
The code I shared compiles with no er
vsapsai wrote:
> @vsapsai I guess, it's a good sign? :) Do you see how our use case can be
> supported by a trivial and low-risk forward fix? If not, I'd insist on a
> revert before we can figure out the way forward. We can run this sort of a
> change through our testing before it relands, and
alexfh wrote:
> Interesting, changing `private header "wrap_foo.h"` to `header "wrap_foo.h"`
> in `b.wrap_foo` stops reproducing the error. I'm not sure it guarantees
>
> > whenever resolving module for the header, pick one that has the header as
> > modular over textual.
>
> But seems we are
vsapsai wrote:
Interesting, changing `private header "wrap_foo.h"` to `header "wrap_foo.h"` in
`b.wrap_foo` stops reproducing the error. I'm not sure it guarantees
> whenever resolving module for the header, pick one that has the header as
> modular over textual.
But seems we are closer to th
vsapsai wrote:
It's weird that clang picked `b.wrap_foo` for "wrap_foo.h" as it is specified
as `private header`. To me it looks like circumventing "use of private header
from outside its module [-Wprivate-header]".
https://github.com/llvm/llvm-project/pull/138227
_
ilya-biryukov wrote:
> What is your timeframe for stopping putting the same header ("wrap_foo.h")
> into multiple modules? If you do that it was never guaranteed which module
> would be used for a header. I'm willing to unblock you but I'd like to know
> if you are committed to fixing the issu
vsapsai wrote:
> I know what we do is cheesy and we might need to reconsider how to approach
> it, but this is a breaking change and it would be great to find a resolution
> for it before committing to the new behavior. Could we revert this while we
> discuss the potential ways out of it?
Wha
ilya-biryukov wrote:
Here's a reproducer for our breakage:
```cpp
// RUN: rm -rf %t
// RUN: split-file %s %t
//
// First, build a module with a header.
//
// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/modules1.map -fmodule-name=a
-emit-module -xc++ -fmodules-embed-all-files -o %t/a.pcm %t/mo
bgra8 wrote:
We have bisected build breakages internally at google at this change.
Ironically the compiler complains about a missing header while before it didn't.
We do not have a reproducer yet.
https://github.com/llvm/llvm-project/pull/138227
___
https://github.com/vsapsai closed
https://github.com/llvm/llvm-project/pull/138227
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai wrote:
Thanks for the review!
https://github.com/llvm/llvm-project/pull/138227
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/Bigcheese approved this pull request.
Just looking at the `missing_textual_header` module alone this is a bit odd,
but that's just the semantics of textual headers. Really textual headers just
exist to control the non-modular include warning, so this is fine.
https://github.
vsapsai wrote:
The change for `exclude` headers was done in
040e12662a674e2ebfc93f86a70eddb7d6fc76da.
https://github.com/llvm/llvm-project/pull/138227
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listi
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: Volodymyr Sapsai (vsapsai)
Changes
According to the documentation
> A header declaration that does not contain `exclude` nor `textual`
specifies a header that contributes to the enclosing module.
Which means that `exclude` and `te
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Volodymyr Sapsai (vsapsai)
Changes
According to the documentation
> A header declaration that does not contain `exclude` nor `textual`
specifies a header that contributes to the enclosing module.
Which means that `exclude` and `textual` h
https://github.com/vsapsai created
https://github.com/llvm/llvm-project/pull/138227
According to the documentation
> A header declaration that does not contain `exclude` nor `textual` specifies
> a header that contributes to the enclosing module.
Which means that `exclude` and `textual` header
38 matches
Mail list logo
