429362 - large unsigned integer constants not passed to JS properly

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Vlad Sukhoy]

On Linux dehydra passes something weird to JS which may make sense within the C context but is hardly appropriate for the JS analyses.

testcase:
void hi() { unsigned int bye=4294967295u; } 

test.js output (with print's ucommented):
{statements:[{name:"bye", type:{name:"unsigned int"}, loc:"simple.cc:1:26", isDecl:true, assign:[{value:"-1u"}]}], loc:"simple.cc:1:9"}

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

Here's a patch with 4 unit tests. It fixes this problem. It also makes -10 be printed out as "-10" instead of the "-0x0000000a" you get from GCC expr_as_string. 

This seems like a bug in the GCC pretty-printer. Should I report it to them?

Comment 2

[(dormant account)]

> This seems like a bug in the GCC pretty-printer. Should I report it to them?

Yes. 
 

Comment 3

[(dormant account)]

Comment on attachment 316123 [details] [diff] [review]
Proposed patch

>+/* Convert an INTEGER_CST to a string representation. This is used
>+ * because GCC expr_as_string is broken for unsigned ints. */
>+const char *dehydra_intCstToString(tree int_cst) 
>+{
>+  static char buf[32];  // holds repr of up to 64-bit ints
>+  xassert(TREE_CODE(int_cst) == INTEGER_CST);
>+  long high = TREE_INT_CST_HIGH(int_cst);
>+  int is_unsigned = TYPE_UNSIGNED(TREE_TYPE(int_cst));
>+  if (high == 0 || high == -1 && !is_unsigned) {
>+    /* GCC prints negative signed numbers in hex, we print using %d.
>+       GCC prints unsigned numbers as if signed, we really do unsigned. */
>+    char *fmt = is_unsigned ? "%uu" : "%d";
>+    sprintf(buf, fmt, TREE_INT_CST_LOW(int_cst));
>+    return buf;
>+  } else {
>+    return expr_as_string(int_cst, 0);
>+  }
> }
> 

This is a general problem in Dehydra. INTEGER_CST nodes are a pain to deal with. Should just make a long long intcstValue(tree). This is needed in many places in the code. Of course most code gets by dandy since typically the LOW part is the only part that is filled in.
Perhaps we should skip the string conversion and make it a JS float value?

Also note that GCC accessor macros such as TREE_INT_CST_HIGH can check the code too, so no need for the xassert in there.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> This is a general problem in Dehydra. INTEGER_CST nodes are a pain to deal
> with. Should just make a long long intcstValue(tree). This is needed in many
> places in the code. Of course most code gets by dandy since typically the LOW
> part is the only part that is filled in.
> Perhaps we should skip the string conversion and make it a JS float value?

Dunno. This issue is pretty painful: a long long with bit 63 set would be ambiguous, it could represent either a long long or an unsigned long long, with different values. JS float loses information for big 64-bit ints, but it would be OK with me. We could also represent as (C type, string repr), which would be unambiguous. In practice, I'm not sure how much the really big numbers matter. (But the GCC pretty printer clearly has annoying behavior even for "normal" numbers like -10.)

Comment 5

[Vlad Sukhoy]

For big 64-bit ints we could e.g. pass a decimal string in value and also pass high/low parts in, say, .value_high and .value_low so that a hypothetical analysis could deal with them without having to extract them from a string representation. 

Comment 6

[(dormant account)]

after the irc conversations, what are we doing about this bug?

Comment 7

[Vlad Sukhoy]

Well, 64-bit ints are still broken with attachment 316123 [details] [diff] [review]. I folded it in the patch on bug 429031 which uses printf extension and completely avoids gcc pretty printer for integer literals. The extra 64-bit testcases are also in that patch.

Comment 8

[(dormant account)]

Closing this bug as the stuff is being solved in bug 429031