anakryiko added a comment. In D131012#3697000 <https://reviews.llvm.org/D131012#3697000>, @aaron.ballman wrote:
> In D131012#3696810 <https://reviews.llvm.org/D131012#3696810>, @anakryiko > wrote: > >> In D131012#3696327 <https://reviews.llvm.org/D131012#3696327>, >> @aaron.ballman wrote: >> >>> In D131012#3695110 <https://reviews.llvm.org/D131012#3695110>, @anakryiko >>> wrote: >>> >>>> This will severly break BPF users, as we have a heavy reliance on `const >>>> volatile` variables being allocated to `.rodata`, which are passed to BPF >>>> verifier in Linux kernel as read-only values. This is critical for BPF >>>> Complie Once - Run Everywhere approach of writing portable BPF >>>> applications. We've been relying on this behavior for years now and >>>> changes this will break many existing BPF applications. >>> >>> Thank you for this feedback! I guess I'm a bit surprised given the contents >>> from the issue seem to imply that BPF needed Clang's behavior to change: >>> `Note that this is causing practical difficulties: the kernel's bpftool is >>> generating different skeleton headers for BPF code compiled from LLVM and >>> GCC, because the names of the containing sections get reflected.` >> >> GCC folks are trying to make their BPF backend usable. But instead of >> working with community to make sure they do things the same way that Clang >> does (which for years now is de facto standard for compiling BPF code and we >> rely on this behavior heavily in libbpf and other BPF loader libraries), >> they instead work against BPF community and try to modify/adjust/break the >> world around them, instead of working with us to make GCC-BPF compatible >> with Clang. > > Ah, that's background information I was unaware of. Thank you for sharing > that perspective! And thank you for holding of on this one and trying to clarify this with WG14! >>> That said, I'm asking on the WG14 reflectors whether there's a normative >>> requirement here or not, so I'm marking this as having planned changes >>> until I hear back from WG14 and until we've resolved whether the changes >>> will fix code vs break code (or both!) so we don't accidentally land this. >> >> Thanks! But note that `const volatile` being in `.rodata` is a very explicit >> expectation in BPF world, so changing that to `.data` will immediately break >> a bunch of different BPF applications that rely on this for BPF CO-RE >> (Compile Once - Run Everywhere) for guarding BPF code that shouldn't work on >> old kernels. .rodata is reported to BPF verifier as fixed, read-only, known >> values and BPF verifier is using those values for control flow analysis and >> dead code elimination. This is crucial feature. > > Yes, but if the standard has a normative requirement in this area, we'd still > have to consider how to move forward in a conforming C mode (I'd guess the > most clear path would be a compiler flag to get the old behavior back again > for those who need it). But that said... > > The responses I've been getting back on the WG14 reflectors are not > indicating that there's any normative requirement. It sounds like the > sentiment is generally that the footnote is trying to tell you that objects > which are `const` qualified but not `volatile` qualified can be assumed to > not change value. Based on that sentiment from the committee, we're under no > obligation to make a change here for conformance to C. > > Where we go from here regarding > https://github.com/llvm/llvm-project/issues/56468 is something we still need > to work out. From what I'm hearing, it sounds like changing Clang will break > a bunch of currently working, valid code. We obviously want to avoid that. > Does anyone have visibility into the GCC community's thoughts and efforts in > this space? Like, are they in the same boat where changing their behavior > will break a bunch of currently working, valid code (outside of BPF) as well? I don't know how hard it is, but at least for GCC's BPF backend they'd need to emit `const volatile` into .rodata to be BPF CO-RE-compatible. Let's continue discussion on Github issue, James and Jose both seem to be involved in this from GCC side, so you already have relevant people there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131012/new/ https://reviews.llvm.org/D131012 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
