@tstellar Thanks for your comments. The MachineInstr SSA based optimization is really a ugly hack. Currently verifier is not able to verify the program correctly, i.e., rejects the program for certain patterns related subregister uses. The commit message provides more information. @ast suggested to look at whether we can improve kernel verifier and I will take a look there. If kernel can be enhanced, I will drop the second MachineInstr SSA pass.

Added a machineinstr SSA target phase to add left/aright shift operations if the 32bit subregister is used for sle/slt/sge/sgt operations.

fix potential overflow issues

Indeed tried kernel selftests, revert the previous workaround like

diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
index d22e438198cf..608a06871572 100644
--- a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
+++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
@@ -44,10 +44,7 @@ int sysctl_tcp_mem(struct bpf_sysctl *ctx)
        unsigned long tcp_mem[TCP_MEM_LOOPS] = {};
        char value[MAX_VALUE_STR_LEN];
        unsigned char i, off = 0;
-       /* a workaround to prevent compiler from generating
-        * codes verifier cannot handle yet.
-        */
-       volatile int ret;
+       int ret;

The current kernel did not handle signed compare well for 32bit registers. I will need to make that change first in order to make this llvm change effective.

To handle cases (exposed when running linux kernel samples/bpf/sockex1) like below:

unsigned long long load_byte(void *skb, unsigned long long off) asm("llvm.bpf.load.byte");
unsigned long long test(void *skb) {
  return load_byte(skb, 10);
}

previous approach emits load_byte as load_byte function is in module function reference list.
But actually this function is not called so we should not emit extern func BTF for this.

use the same XADD opcode for new instructions.

implement exchange instruction

add a test case for extern function with arguments.

@dblaikie The root cause has been identified by @rnk . A clang plugin is used which requires the following patch:

https://github.com/llvm/llvm-project/commit/5128026467cbc17bfc796d94bc8e40e52a9b0752

which has been merged. The original extern variable debug info type has been relanded. So we are good. Thanks!

Could you link to particular bots that have logs showing they ran this test? (perhaps the logs have been retired by now, though - since this patch was reverted :/ - but then at least seeing which bots run BPF tests would be helpful)

@rnk Great to know the reason. Thanks!

@rnk I somehow cannot reproduce the issue locally. If you can let me know how to exactly reproduce the issue, I might be able to also help with some kind of solution, although frankly speaking, I am not a clang expert.

Removed REQUIRES: bpf-registered-target. This is not needed any more if using %clang_cc1. Tested by build clang with and without BPF target.

@rnk I just submitted a patch https://reviews.llvm.org/D71818 to use %clang_cc1 instead of %clang in the test. Could you check whether this fixed your issue or not?

I cannot remove BPF target requirement as only BPF target can trigger debug info generation for extern variables. This is by design as discussed in D70696 due to concerns it may blow up debug info binary size for existing applications. This may be relaxed in the future through some flags.
BTW, as far as I know, BPF is enabled for a lot of bots, ubuntu, x86, ppc, arm, etc. So the coverage of this future should be fine.

use bpf_type.info lower two bits to encode func scope info

merged.

remove some redundant test cases

@Jac1494 Based on early discussion, we cannot just issue debuginfo for externs when -g is specified as this may cause debuginfo (and final binary) size bloat. One possible way is to enable it when -fstandalone-debug is specified. @dblaikie can you comment on this patch and general possible forward path for extern variable debug info support?

extern variables can have attribute((weak)) attribute. Support it as well.

@dblaikie I consolidated test cases as you suggested. Along the way, I also added some more tests. I already answered some of your questions earlier. Please let me know what you think.

consolidated into less test files, added more test cases along the way.

In D70696#1767616, @dblaikie wrote:

Many of the test cases could be collapsed into one file - using different variables that are used, unused, locally or globally declared, etc.

clang-format change (from @aprantl) , add a little details into summary related to recent discussion,
move tests from CodeGenCXX to CodeGen as they are all C tests.

@dblaikie The concern for that users using -fstandalone-debug may suddenly see a bloat of external var debug info is totally valid. Let me just stick to BPF use case for now. If somebody else ever wants this information with -fstandalone-debug, the restriction can be evaluated and relaxed then.

@dblaikie Good points. I will guard external variable debug info generation under -fstandalone-debug flag.

@probinson for the question,

Does bpf require debug info for the declaration of global_var in noref.c ?

No, bpf only cares the referenced external global variables. So my current implementation does not emit debug info
for external global_var in noref.c.

@SouraVX Currently, I filtered all external globals if they are not used. See

for (auto D: ExternalDeclarations) {
  if (!D || D->isInvalidDecl() || D->getPreviousDecl() || !D->isUsed())
    continue;

additional test case is added to ensure debuginfo for externs is indeed generated in BTF.

The backend supports extern variable debug info as well. The following restrictions in the old code is removed.

 -    // Ignore external globals for now.
-    if (!Global.hasInitializer() && Global.hasExternalLinkage())
-      continue;

@dblaikie As suggested, just submitted two separate patches:

• https://reviews.llvm.org/D70696 for clang/llvm change to generate debuginfo for extern variables
• https://reviews.llvm.org/D70697 for BPF backend to consume such debuginfo.

The corresponding BPF patch to consume the debuginfo https://reviews.llvm.org/D70697.

The corresponding clang/llvm patch which generates debuginfo for extern variables. https://reviews.llvm.org/D70696

@dblaikie Thanks for the review. To address your comments below:

This patch is not needed any more.

The patch is merged. Sorry I forgot to add the link to the patch.

@tstellar
About the impact of this bug:

• to trigger the bug, the additional additional -mattr=+alu32 is needed.
• with this bug, the program is most likely to be rejected by kernel verifier (based on what we know), so people likely just not using -mattr=+alu32 to make kernel verifier happy

That said, it would be still good to backport the change to 9.0.x if possible.

Look into the implementation in BPF backend.

@spatel Thanks for the comments. I agree with your assessment. Looks like the best place is BPF backend to undo this optimization. I will look into implementation in that direction.

I added a few reviewers based on the recent commits at InstCombine directory. Please let me know if I missed anybody who should review the patch. Thanks!

Sorry about the committing. I thought all the review questions are addressed.
Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.
Thanks for the detailed review! I have addressed all of your review comments in patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.

@aaron.ballman just ping, could you let me know if you have any further comments? Thanks!

@aaron.ballman BTW, I indeed tested C-style inheritance. An attribute for the forward declaration successfully inherited by a later actual declaration.

Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
(a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
template instantiation since we are at clang stage?

Hi, @aaron.ballman I think I addressed all your comments, could you take a look again? I tested all failed tests as exposed by the buildbot here (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/57856). They are all passing now. Thanks!

use implicit attr to annotate records or fields not explicitly marked by users.

The Decl "D" could be a nullptr in ProcessDeclAttributeDelayed, which will cause segfault. Handle this properly.
Also addressed most of Aaron's comments.