Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CIR][CIRGen][TBAA] Struct with Union crashes during CodeGen #1246

Closed
bruteforceboy opened this issue Dec 19, 2024 · 5 comments · May be fixed by #1250
Closed

[CIR][CIRGen][TBAA] Struct with Union crashes during CodeGen #1246

bruteforceboy opened this issue Dec 19, 2024 · 5 comments · May be fixed by #1250
Assignees
@PikachuHyA

Comments

@bruteforceboy
Copy link
Contributor

The following code snippet crashes when generating CIR with -O2 enabled after PR#1220 was merged.

typedef struct {
  union {
    int a, b;
  };
  int c;
} S;

void foo(S *s) { s->a = 1; }

I haven't done a thorough review of the changes made, but I just noticed the PR broke something that worked for me before. Can't we add unions here, or just emit tbaa_NYI for all the non-scalar types for now?

cc: @PikachuHyA, @bcardosolopes
@PikachuHyA PikachuHyA self-assigned this Dec 20, 2024
@PikachuHyA PikachuHyA mentioned this issue Dec 20, 2024
Open
@PikachuHyA
Copy link
Collaborator

hi @bruteforceboy

Thank you for bringing this to my attention. I sincerely apologize for the issues caused by my recent patch #1220 .

You're correct that the changes affected the handling of unions, leading to the crash you experienced. I've fixed in #1250 .

Thanks again for your feedback, and please let me know if you encounter any further problems or have additional suggestions.

@bruteforceboy
Copy link
Contributor Author

Hi @PikachuHyA! thanks alot for fixing it, I will test the patch soon. It wasn't a serious issue by the way 😅 I was just bringing it to your attention.

@PikachuHyA
Copy link
Collaborator

Can't we add unions here, or just emit tbaa_NYI for all the non-scalar types for now?

follow the skeleton of OG, the union is marked as omnipotent char now

run ./bin/clang t.c -O1 -Xclang -emit-llvm -o t.orig.ll -c

; ModuleID = 't.c'
source_filename = "t.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write) uwtable
define dso_local void @foo(ptr nocapture noundef writeonly initializes((0, 4)) %s) local_unnamed_addr #0 {
entry:
  store i32 1, ptr %s, align 4, !tbaa !5
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write) uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3}
!llvm.ident = !{!4}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 8, !"PIC Level", i32 2}
!2 = !{i32 7, !"PIE Level", i32 2}
!3 = !{i32 7, !"uwtable", i32 2}
!4 = !{!"clang version 20.0.0git"}
!5 = !{!6, !6, i64 0}
!6 = !{!"omnipotent char", !7, i64 0}
!7 = !{!"Simple C/C++ TBAA"}

run ./bin/clang t.c -O1 -Xclang -emit-llvm -o t.ll -c -fclangir with #1250

; ModuleID = '/disk8/shuoshu.yh/admin_debug/acc_clangir_github/build/t.c'
source_filename = "/disk8/shuoshu.yh/admin_debug/acc_clangir_github/build/t.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write) uwtable
define dso_local void @foo(ptr nocapture writeonly initializes((0, 4)) %0) local_unnamed_addr #0 {
  store i32 1, ptr %0, align 4, !tbaa !2
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write) uwtable }

!llvm.module.flags = !{!0, !1}

!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = !{i32 7, !"uwtable", i32 2}
!2 = !{!3, !3, i64 0}
!3 = !{!"omnipotent char", !4, i64 0}
!4 = !{!"Simple C/C++ TBAA"}

@bcardosolopes
Copy link
Member

@PikachuHyA that has been my point about crashing valid code, TBAA should be added incrementally without causing churn :)

I'll revert #1220 for now, thanks for the report.

@bcardosolopes
Copy link
Member

I had to revert both 1220 and 1242. @PikachuHyA I suggest you don't re-add 1242 until we fix the long long problem, because it might be a bit off depending on what we decide (thanks for working on that though, pretty cool tablegen solution). I suggest you put a new PR for 1220 with the minimum necessary part of #1250 to fix the problem, so we can reland faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PikachuHyA PikachuHyA

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[CIR][CIRGen][TBAA][NFC] Skeleton for union, enum and bitint PikachuHyA/clangir
3 participants
@bcardosolopes @bruteforceboy @PikachuHyA