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

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

Comments

@samuelsadok
Copy link
Mannequin

@samuelsadok 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 samuelsadok mannequin commented Jun 8, 2021

assigned to @rotateright

@efriedma-quic
Copy link
Collaborator

@efriedma-quic efriedma-quic commented Jun 8, 2021

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

@rotateright rotateright commented Jun 9, 2021

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

@rotateright rotateright commented Jun 9, 2021

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 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 tstellar commented Jun 9, 2021

Merged: b54ccef

@samuelsadok
Copy link
Mannequin Author

@samuelsadok samuelsadok mannequin commented Jun 10, 2021

Thanks for the quick response time!

@jyknight
Copy link
Member

@jyknight jyknight commented Nov 27, 2021

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

@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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants