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

CBE miscompilation of 197.parser #1471

Closed
lattner opened this issue Jan 9, 2007 · 7 comments
Closed

CBE miscompilation of 197.parser #1471

lattner opened this issue Jan 9, 2007 · 7 comments
Labels
bugzilla Issues migrated from bugzilla miscompilation regression

Comments

@lattner
Copy link
Collaborator

lattner commented Jan 9, 2007

Bugzilla Link 1099
Resolution FIXED
Resolved on Feb 22, 2010 12:51
Version trunk
OS All

Extended Description

Reduced by bugpoint. The issue is that this testcase:

target datalayout = "e-p:32:32"
target endian = little
target pointersize = 32
target triple = "i686-apple-darwin8"
%struct.Connector = type { i16, i16, i8, i8, %struct.Connector*, i8* }

implementation ; Functions:

define bool %prune_match_entry_2E_ce(%struct.Connector* %a, i16 %b.0.0.val) {
newFuncRoot:
br label %entry.ce

cond_next.exitStub: ; preds = %entry.ce
ret bool true

entry.return_crit_edge.exitStub: ; preds = %entry.ce
ret bool false

entry.ce: ; preds = %newFuncRoot
%tmp = getelementptr %struct.Connector* %a, i32 0, i32 0 ; <i16*> [#uses=1]
%tmp = load i16* %tmp ; [#uses=1]
%tmp = icmp eq i16 %tmp, %b.0.0.val ; [#uses=1]
br bool %tmp, label %cond_next.exitStub, label %entry.return_crit_edge.exitStub
}

is compiled to:

struct l_struct_2E_Connector {
signed short field0;
signed short field1;
signed char field2;
signed char field3;
struct l_struct_2E_Connector *field4;
signed char *field5;
};
...
bool prune_match_entry_2E_ce(struct l_struct_2E_Connector *ltmp_0_1, unsigned short ltmp_1_2) {
signed short ltmp_2_2;

ltmp_2_2 = *(&ltmp_0_1->field0);
return ((((ltmp_2_2 == ltmp_1_2)) ? (1) : (0)));
}

Note that the == is comparing a signed short and an unsigned short. These both get extended
(differently) to int before the comparison is done.

-Chris

@lattner
Copy link
Collaborator Author

lattner commented Jan 9, 2007

One solution would be to emit:

ltmp_2_2 == (signed short)ltmp_1_2

instead of

ltmp_2_2 == ltmp_1_2

-Chris

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 9, 2007

For the record, this is the bugpoint command that reduced this test case:

cd Output/bugpoint-test
bugpoint ../197.parser.llvm.bc -cbe-bug -append-exit-code -Xlinker=-lm
-input=test.in -output=../197.parser.out-nat -timeout=500
--tool-args --args -- 2.1.dict -batch

@lattner
Copy link
Collaborator Author

lattner commented Jan 9, 2007

yes, which assumes that you primed it with "make clean; make RUN_TYPE=test bugpoint-llc" then hit ctrl-
c a few times once bugpoint started running.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 9, 2007

Hmm. This seems to be fallout from parameter attributes change.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 9, 2007

This patch:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070108/042265.html

is a partial fix for this PR. It defaults function parameters to signed
integer, just like everything else in CBE. The bug was caused by incorrectly
introducing parameter attributes feature by choosing "signed" parameter if the
SExtAttribute was specified. Howeer, if no attribute is specified, this
causes it to become unsigned which is incorrect. Reversing the logic so
that signedness is detected by "not ZExtAttribute set" fixes many cases.

This fixes 197.parser but there is more to do. Any comparison and possibly
other operators involving arguments may need to correctly cast the parameter
before its use, depending on the sign of the operator.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 9, 2007

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 26, 2021

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

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
clementval added a commit to clementval/llvm-project that referenced this issue Feb 16, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla miscompilation regression
Projects
None yet
Development

No branches or pull requests

2 participants