Rebase, will merge this in after CI run.

Thank you for all the feedback!
I've applied the latest suggestions and going to merge this revision once CI is done.

Changes in accordance with @MaskRay feedback.

Could you please take a look at this revision?
I think I addressed all feedback.

Fix for failed unit test pragma-attribute-supported-attributes-list.test

• Use (new) attribute((preserve_static_offset)) instead of attribute((btf_decl_tag("ctx"))).
• Rename files, passes, etc accordingly.

Missing dot at the end of the comment.

Unit tests are passing on S390 under emulation w/o issues.

Thank you for the feedback, I think I covered all but one items.
The last remaining item is to verify that unit test is working on the bigendian system.
I'll update the revision once I check that.

Stylistic changes according to feedback from @MaskRay and @jhenderson.

In D133361#4629292, @ast wrote:

...
Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot do it. Always propagating it won't be correct.
New preserve_const_field_offset would need to be propagated too and we actually have nested unions.
__bpf_md_ptr is such example. btf_tag wouldn't propagate into that union, but attr(preserve_const_field_offset) should.

After retesting kernel build with LLVM=1 and libbpf patch to reconstruct btf_decl_tag [1], statistics for BPF selftests looks as follows:

• out of 653 object files 13 have some differences with and w/o this change;
• for 2 programs there is small instruction count increase (+2 insn total);
• for 5 programs there is small instruction decrease (-6 insn total);
• 6 programs differ slightly but number of instructions is the same.

In D133361#4628951, @ast wrote:

I agree that it's not useful outside of BPF, but it's useful outside of 'ctx'. I think 'preserve_constant_field_offset' would be more accurate description of the restriction.
We can expand in the doc that it's a constant offset when field of the struct is accessed.

Also instead of btf_tag it would be better to add another builtin similar to preserve_access_index.
Currently we add attribute((preserve_access_index)) to trigger CO-RE.
This one will be a new attribute((preserve_constant_field_offset)) that will be specified manually either in uapi/bpf.h or in vmlinux.h on some structs

git-clang-format fixes.

• Removed special Sema processing and btf_decl_tag("ctx") prioritization in CGExpr, preserve access index calls are now handled by BPFContextMarker transformation.
• Correctness fixes for isPointerOperand().

In D150079#4608909, @MaskRay wrote:

Sorry, was pretty busy handling a backlog of reviews and day work... Feel free to ping after a week https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted

Fixes basing on feedback from @MaskRay.
Fix for incorrect BTF::CommonType::getVlen() mask.

Rebase, want to see green CI build

If you have some time, could you please take a look at my response?

If you have some time, could you please take a look?

Hi Yonghong,

By the way, it is still possible to test this using e.g. llc or clang. As noted previously in this thread the default error handler for llc/clang is stateful, so it proceeds the code generation, produces assembly code and returns non-zero error code.

Hi Yonghong, could you please take a look?
As described here, this fixes one of the selftest compilation bugs that occur when LLVM_ENABLE_EXPENSIVE_CHECKS is on. Sorry for the long description, I tried to explain why instruction selection would always cover cases that BPFMIPeepholeTruncElim covered.

As an additional data point, the same example but w/o section specification compiles fine:

After this revision landed yesterday one BPF kernel selftest (written in C) stopped building. I narrowed the issue to the following example:

In D157806#4583421, @yonghong-song wrote:

... Another solution is similar to D157805 where we can clear 'killed' for the 'out' parameter, right?

In D157805#4583287, @yonghong-song wrote:

LGTM. Thanks for detailed comments to show why generated code is incorrect. Do you think we should backport this fix to llvm17?

The build bot failure (here) was caused by newly added test case field-reloc-st-imm.ll. When LLVM is built with LLVM_ENABLE_EXPENSIVE_CHECKS=ON flag this test triggered bug described in D157806. I rebased this revision on top of D157806. Could you please double check if you are still ok with these changes?

I have finished changes for this revision, please take a look.

Rebased on top of D157806.

It is possible to avoid separate versions for CORE_ST32 and CORE_ST64, merging these simplifies implementation for D140804.

Hold-on, I need to update this one to be more in line with changes necessary for D140804 functionality.

Hi Yonghong,

Hi Yonghong,

Build bot reports a failure (link):

Could you please take a look at my response?

Changes according to Yonghong's comments

Note: CI failures seem to be completely unrelated...

Hi Yonghong,

I messed up.
@ast is right, I missed the fact that clang already exits with return code 1 when any of the conditions reported by fail(...) happen, e.g.:

After examining source code for llvm-lit, I found an option --crash for not. Here is the portable incantation:

In D156497#4554932, @tamird wrote:

Note also that 8e6aab3 (D21368) removed the exit-on-error flag which is necessary for the unit tests with this change.

I think that even if everyone agreed that crashing is acceptable we'd have to find a solution that would allow the tests to pass. Any ideas?

I grepped the code base and see that finalizeLowering() logic is customized only in a handful of cases:

• llvm/lib/Target/ARM/ARMISelLowering.cpp
• llvm/lib/Target/AMDGPU/SIISelLowering.cpp
• llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

In D156446#4554385, @tamird wrote:

Sounds good! I'll do that. Can you share the C source code of the program as well please?

In D156445#4554360, @tamird wrote:

In D156445#4554232, @eddyz87 wrote:

In my opinion, whether or not to repeat MI->getOperand(NumDefs) is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.

I seems obvious that readability of code-at-HEAD should be of a higher priority than the readability of the commit history; the extant code will be read many more times than the the commit history for the same piece of code.

In my opinion, whether or not to repeat MI->getOperand(NumDefs) is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.

This error could be detected using the following test case:

Thank you for the detailed response!

In D156136#4545957, @tamird wrote:

Could someone land this for me please?

I left a few minor comments.
All-in-all I think these modifications are fine if it makes BPF backend simpler to work with.
However, I'm not sure if fixing my comments is worthwhile if Alexei objects the whole idea.

In D155894#4540874, @tamird wrote:

In D155894#4529382, @eddyz87 wrote:

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

Doesn't this still run at the end of a function, rather than at the end of an entire file? We can report multiple functions have errored, but we can't emit all the errors in the file.

if you have a minute, could you please take a look?
I used this locally and it proved to be convenient for debugging (e.g. here).

Could you please take a look at my reply?

To better reiterate my position on the warning for ExternalSymbolSDNode: in C analogy such warnings would be generated by the linker, not compiler.
So, I vote to agree with @tamird here and remove the warning (or at-least make it optional).

In D155894#4532046, @tamird wrote:

Figured it out - if you pass --bpf-expand-memcpy-in-order to llc, you will reproduce the A call to built-in function 'memset' is not supported. prints.

Hi Yonghong,

In D155894#4529918, @tamird wrote:

Yeah, I also can't reproduce using llc. But in the actual code this module is fed into LLVMTargetMachineEmitToFile and those errors do show up.

In D155894#4529763, @tamird wrote:

In D155894#4529756, @eddyz87 wrote:

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

Yes, please, that would be helpful.

https://gist.github.com/tamird/f3ff414f183b085c3e37cd7d5b5754de

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

In D155894#4528546, @tamird wrote:

• change of the behavior for ExternalSymbolSDNode: this was not a part of Diff 1, so I assume that it was not a part of the @tamird's initial intent. Is there a use case for this change? The libbpf commit 0a34245 deals only with external maps, not external functions.

When compiling Rust code, we see this warning trigger on memset intrinsics, but this is not a problem because we provide an implementation of memset and the intrinsic is eventually lowered to a call to the function we provide. Is there a finer-grained change that would achieve our goals?

Hi Yonghong,

Please note that a number of unit tests fails with this patch applied:

• LLVM :: CodeGen/BPF/many_args1.ll
• LLVM :: CodeGen/BPF/many_args2.ll
• LLVM :: CodeGen/BPF/struct_ret1.ll
• LLVM :: CodeGen/BPF/vararg1.ll
• LLVM :: CodeGen/BPF/warn-call.ll

I tried adding a test similar to assemble-disassemble.ll:

Could you please take a look at this revision?
It adds support for CO-RE relocations printing in llvm-objdump.
While current implementation works, there are two aspects that might be sub-optimal:

• ELF relocations are not unified with BTF relocations and thus there are two functions: llvm-objdump.cpp:printRelocation(), llvm-objdump.cpp:printBTFRelocation();
• The BTFParser instance is not shared between LLVMSymbolizer (used for source line printing) and llvm-objdump.cpp:disassembleObject() (used for relocations printing).

In D155176#4497053, @luporl wrote:

I've tested this patch and it also fixes unit tests for this ARM bot:
https://lab.llvm.org/buildbot/#/builders/245/builds/10973

I opened D155176 with packed attribute shuffling, will commit after pre-merge checks are complete for it.
(Want to make sure that the change does not cause issues with MSVC).

In D149058#4495293, @MaskRay wrote:

The test has good test coverage for the regular code path, but the error code paths need the unittest.

Sorry, I was away from the PC. Thank you for committing the fix, I will update the unit test to execute in both big and little endian.
However, it looks like there is one more issue, as reported here for address sanitizer:

Changes requested by @MaskRay in the last review round.