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

[TBAA][DSE] bitfield store is incorrectly optimized out #82586

Closed
dtcxzyw opened this issue Feb 22, 2024 · 2 comments · Fixed by #82922
Closed

[TBAA][DSE] bitfield store is incorrectly optimized out #82586

dtcxzyw opened this issue Feb 22, 2024 · 2 comments · Fixed by #82922
Assignees
@fhahn @dobbelaj-snps
Labels
miscompilation TBAA Type-Based Alias Analysis / Strict Aliasing

Comments

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 22, 2024

Reproducer: https://godbolt.org/z/jKrGW5jEh

#include <stdint.h>

struct S1 {
   signed f0 : 9;
   unsigned f1 : 2;
   int8_t  f2;
};

struct S1 g;

struct S1 func(struct S1* x) {
    x->f2 = 3;
    *x = g;
    return *x;
}

x->f2 = 3 is incorrectly eliminated by DSEPass.
This miscompilation is caused by recent commit #81313.
@dtcxzyw dtcxzyw added miscompilation TBAA Type-Based Alias Analysis / Strict Aliasing labels Feb 22, 2024
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Feb 22, 2024

Another reproducer: https://godbolt.org/z/Y3bnsMqd7

@dtcxzyw dtcxzyw mentioned this issue Feb 22, 2024
Open
@fhahn
Copy link
Contributor

fhahn commented Feb 22, 2024

Let me take a look. It seems like clang emits tbaa .struct with overlapping fields. Need to check what exactly is going on

fhahn added a commit that referenced this issue Feb 25, 2024
@fhahn
[TBAA] Test for tbaa.struct creation for struct with named bitfields.
7110147 
Add test for tbaa.struct metadata creation for copies of a struct with
named bitfields.

Test for #82586.
fhahn added a commit to fhahn/llvm-project that referenced this issue Feb 25, 2024
@fhahn
[TBAA] Skip all bitfields when generating !tbaa.struct metadata.
556fcef 
At the moment, clang generates what I believe are incorrect !tbaa.struct
fields for named bitfields. At the moment, the base type size is used
for named bifields (e.g. sizeof(int)) instead of the bifield width per
field. This results in overalpping fields in !tbaa.struct metadata.

This causes incorrect results when extracting individual copied fields
from !tbaa.struct as in added in dc85719.

This patch fixes that by skipping all bitfields, not only unnamed ones
(note that CollectFields has a TODO to support bitfields). As bitfields
specify their widths in bits, while !tbaa metadata uses bytes for sizes
and offsets, I don't think we would be able to generate correct metadata
for them in general.

If this understanding is correct, I can also extend the verifier to
check that !tbaa.struct fields aren't overlapping.

Fixes llvm#82586
@fhahn fhahn mentioned this issue Feb 25, 2024
Merged
fhahn added a commit that referenced this issue Feb 27, 2024
@fhahn
[TBAA] Handle bitfields when generating !tbaa.struct metadata. (#82922)
d2a9df2 
At the moment, clang generates what I believe are incorrect !tbaa.struct
fields for named bitfields. At the moment, the base type size is used
for named bifields (e.g. sizeof(int)) instead of the bifield width per
field. This results in overalpping fields in !tbaa.struct metadata.

This causes incorrect results when extracting individual copied fields
from !tbaa.struct as in added in dc85719.

This patch fixes that by skipping by combining adjacent bitfields
in fields with correct sizes.

Fixes #82586
fhahn added a commit to fhahn/llvm-project that referenced this issue Mar 1, 2024
@fhahn
[TBAA] Test for tbaa.struct creation for struct with named bitfields.
72b3d12 
Add test for tbaa.struct metadata creation for copies of a struct with
named bitfields.

Test for llvm#82586.

(cherry-picked from 7110147)
fhahn added a commit to fhahn/llvm-project that referenced this issue Mar 1, 2024
@fhahn
[TBAA] Handle bitfields when generating !tbaa.struct metadata. (llvm#…
643bc6b 
…82922)

At the moment, clang generates what I believe are incorrect !tbaa.struct
fields for named bitfields. At the moment, the base type size is used
for named bifields (e.g. sizeof(int)) instead of the bifield width per
field. This results in overalpping fields in !tbaa.struct metadata.

This causes incorrect results when extracting individual copied fields
from !tbaa.struct as in added in dc85719.

This patch fixes that by skipping by combining adjacent bitfields
in fields with correct sizes.

Fixes llvm#82586

(cherry-picked from d2a9df2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fhahn fhahn

@dobbelaj-snps dobbelaj-snps

Labels
miscompilation TBAA Type-Based Alias Analysis / Strict Aliasing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[TBAA] Handle bitfields when generating !tbaa.struct metadata. fhahn/llvm-project
3 participants
@fhahn @dtcxzyw @dobbelaj-snps