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.

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

Overly optimized simultaneous copy of struct fields corrupts destination struct #49967

Closed
samuelsadok mannequin opened this issue Jun 8, 2021 · 8 comments
Closed

Overly optimized simultaneous copy of struct fields corrupts destination struct #49967

samuelsadok mannequin opened this issue Jun 8, 2021 · 8 comments
Assignees
@rotateright
Labels
bugzilla Issues migrated from bugzilla llvm:codegen

Comments

@samuelsadok
Copy link
Mannequin

samuelsadok mannequin commented Jun 8, 2021

Bugzilla Link 50623
Resolution FIXED
Resolved on Jun 10, 2021 12:51
Version 12.0
OS Linux
Blocks #48661
CC @dwblaikie,@efriedma-quic,@RKSimon,@zygoloid,@rotateright,@tstellar
Fixed by commit(s) 2ef81cb dd763ac 4232693 b54ccef

Extended Description

Clang 12.0.0 modifies a field of a struct that should not be modified. This is a regression from clang 11.0.1.

Input File:

struct S { char val1; char padding; short val2; };

void func(struct S src, struct S* dest) {
    dest->val1 = src.val1;
    dest->val2 = src.val2;
}

Output from clang 12.0.0 (clang -c test.c -o test.o -O1 && objdump -S test.o -M intel):

0000000000000000 <func>:
   0:	89 3e                	mov    DWORD PTR [rsi],edi
   2:	c3                   	ret

Output from clang 11.0.1:

0000000000000000 <func>:
   0:	40 88 3e             	mov    BYTE PTR [rsi],dil
   3:	c1 ef 10             	shr    edi,0x10
   6:	66 89 7e 02          	mov    WORD PTR [rsi+0x2],di
   a:	c3                   	ret

The output from clang 12.0.0 is incorrect because it modifies the field dest->padding.

See also here: https://godbolt.org/z/dsr9soqbf
"armv8-a clang (trunk)" exhibits the same problem.
@samuelsadok
Copy link
Mannequin Author

samuelsadok mannequin commented Jun 8, 2021

assigned to @rotateright

@efriedma-quic
Copy link
Collaborator

https://reviews.llvm.org/D86420 ?

Slightly more clear testcase:

struct S { char val1; char padding; short val2; };
void func(struct S *dest, int x) {
dest->val1 = x;
dest->val2 = x >> 16;
}

@rotateright
Copy link
Contributor

https://reviews.llvm.org/D86420 ?

Yes...ouch.

That patch allowed matching multiple types, but I failed to make sure that the types match each other and/or map all of the stored bytes.

Working on a fix now.

@rotateright
Copy link
Contributor

Should be fixed after:
https://reviews.llvm.org/rGdd763ac79196

Leaving status as open as a candidate for the 12.0.1 release.

@jyknight
Copy link
Member

jyknight commented Jun 9, 2021

*** Bug llvm/llvm-bugzilla-archive#50635 has been marked as a duplicate of this bug. ***

@tstellar
Copy link
Collaborator

tstellar commented Jun 9, 2021

Merged: b54ccef

@samuelsadok
Copy link
Mannequin Author

samuelsadok mannequin commented Jun 10, 2021

Thanks for the quick response time!

@jyknight
Copy link
Member

mentioned in issue llvm/llvm-bugzilla-archive#50635

@tstellar tstellar mentioned this issue Feb 22, 2021
Closed
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rotateright rotateright

Labels
bugzilla Issues migrated from bugzilla llvm:codegen
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tstellar @jyknight @rotateright @efriedma-quic