Bug 84945 - [8 Regression] UBSAN: gcc/config/i386/i386.c:33312:22: runtime error: shift exponent 32 is too large for 32-bit type 'int'
Summary: [8 Regression] UBSAN: gcc/config/i386/i386.c:33312:22: runtime error: shift e...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 8.0
: P1 normal
Target Milestone: 8.0
Assignee: Jakub Jelinek
URL:
Keywords:
: 84932 (view as bug list)
Depends on:
Blocks: ubsan
  Show dependency treegraph
 
Reported: 2018-03-19 07:54 UTC by Martin Liška
Modified: 2018-04-16 11:23 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-03-19 00:00:00


Attachments
gcc8-pr84945.patch (1.77 KB, patch)
2018-03-19 09:24 UTC, Jakub Jelinek
Details | Diff
64bit const patch (1.66 KB, application/mbox)
2018-03-19 10:28 UTC, Julia Koval
Details
gcc8-pr84945.patch (2.31 KB, patch)
2018-03-19 15:47 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2018-03-19 07:54:21 UTC
Following causes an UBSAN:

$ cat tc.i
void a() {          __builtin_cpu_supports ("gfni"); }


$ ./xgcc -B. tc.i
../../gcc/config/i386/i386.c:33312:22: runtime error: shift exponent 32 is too large for 32-bit type 'int'
    #0 0x2a7434e in fold_builtin_cpu ../../gcc/config/i386/i386.c:33312
    #1 0x2a76664 in ix86_fold_builtin ../../gcc/config/i386/i386.c:33334
    #2 0x10055ba in fold_build_call_array_loc(unsigned int, tree_node*, tree_node*, int, tree_node**) ../../gcc/fold-const.c:12450
    #3 0x73a5b7 in build_function_call_vec(unsigned int, vec<unsigned int, va_heap, vl_ptr>, tree_node*, vec<tree_node*, va_gc, vl_embed>*, vec<tree_node*, va_gc, vl_embed>*) ../../gcc/c/c-typeck.c:3124
    #4 0x7b018f in c_parser_postfix_expression_after_primary ../../gcc/c/c-parser.c:9155
    #5 0x776898 in c_parser_postfix_expression ../../gcc/c/c-parser.c:8980
    #6 0x7998ca in c_parser_unary_expression ../../gcc/c/c-parser.c:7260
    #7 0x79c22a in c_parser_cast_expression ../../gcc/c/c-parser.c:7104
    #8 0x79ca1b in c_parser_binary_expression ../../gcc/c/c-parser.c:6907
    #9 0x79fe76 in c_parser_conditional_expression ../../gcc/c/c-parser.c:6645
    #10 0x7a108b in c_parser_expr_no_commas ../../gcc/c/c-parser.c:6562
    #11 0x7a15fe in c_parser_expression ../../gcc/c/c-parser.c:9292
    #12 0x7aad13 in c_parser_expression_conv ../../gcc/c/c-parser.c:9325
    #13 0x7d43f7 in c_parser_statement_after_labels ../../gcc/c/c-parser.c:5540
    #14 0x7dac3c in c_parser_compound_statement_nostart ../../gcc/c/c-parser.c:5078
    #15 0x7dc332 in c_parser_compound_statement ../../gcc/c/c-parser.c:4912
    #16 0x7df56e in c_parser_declaration_or_fndef ../../gcc/c/c-parser.c:2341
    #17 0x7f9e10 in c_parser_external_declaration ../../gcc/c/c-parser.c:1643
    #18 0x7fbd61 in c_parser_translation_unit ../../gcc/c/c-parser.c:1524
    #19 0x7fbd61 in c_parse_file() ../../gcc/c/c-parser.c:18411
    #20 0x8ebe43 in c_common_parse_file() ../../gcc/c-family/c-opts.c:1132
    #21 0x1d0fdfa in compile_file ../../gcc/toplev.c:455
    #22 0x639d9c in do_compile ../../gcc/toplev.c:2132
    #23 0x639d9c in toplev::main(int, char**) ../../gcc/toplev.c:2267
    #24 0x63c7fa in main ../../gcc/main.c:39
    #25 0x7ffff5cafa86 in __libc_start_main (/lib64/libc.so.6+0x21a86)
    #26 0x63c929 in _start (/home/marxin/Programming/gcc/objdir2/gcc/cc1+0x63c929)
Comment 1 Jakub Jelinek 2018-03-19 08:47:54 UTC
Ugh, this is an important issue.
In GCC 7 we had F_MAX 31, so all of the runtime feature tests are fine.

We have:
struct __processor_model
{
  unsigned int __cpu_vendor;
  unsigned int __cpu_type;
  unsigned int __cpu_subtype;
  unsigned int __cpu_features[1];
} __cpu_model;
and that is an exported variable from libgcc, thankfully only as @GCC_4.8 symbol and thus new programs shouldn't link against that.

No matter what, at least for targets that don't use symbol versioning and prevent using that symbol, I think we need to rename __cpu_model to something else (__cpu_model2), add at least one another 32-bit __cpu_features bitfield and for features >= 32 use the second field instead of the first one.
Comment 2 Jakub Jelinek 2018-03-19 09:24:23 UTC
Created attachment 43696 [details]
gcc8-pr84945.patch

Untested fix.
I think this ought to work well on x86_64/i?86-linux, but I'm afraid is going to be an ABI change otherwise, because __cpu_model exported out of libgcc will change in that case size and objects referencing it might have copy relocations against it.

Another possibility is not to add the extra features into __cpu_model.__cpu_features, but add a new variable (say __cpu_features2), and make sure to not add this variable at all into libgcc_s.so.1 (i.e. force it to be libgcc.a only), then __builtin_cpu_supports for these new features would always link in the libgcc.a(cpuinfo.o).
Comment 3 Julia Koval 2018-03-19 10:28:31 UTC
Created attachment 43698 [details]
64bit const patch

As an idea, can we use 64bit int instead of second array element(was proposed in PR84932)?
Comment 4 Jakub Jelinek 2018-03-19 10:39:29 UTC
See above, that is an ABI change on all of x86.
Comment 5 Jakub Jelinek 2018-03-19 10:40:47 UTC
*** Bug 84932 has been marked as a duplicate of this bug. ***
Comment 6 H.J. Lu 2018-03-19 12:03:33 UTC
(In reply to Jakub Jelinek from comment #2)
> Created attachment 43696 [details]
> gcc8-pr84945.patch
> 
> Untested fix.
> I think this ought to work well on x86_64/i?86-linux, but I'm afraid is
> going to be an ABI change otherwise, because __cpu_model exported out of
> libgcc will change in that case size and objects referencing it might have
> copy relocations against it.
> 
> Another possibility is not to add the extra features into
> __cpu_model.__cpu_features, but add a new variable (say __cpu_features2),
> and make sure to not add this variable at all into libgcc_s.so.1 (i.e. force
> it to be libgcc.a only), then __builtin_cpu_supports for these new features
> would always link in the libgcc.a(cpuinfo.o).

I like this approach better.
Comment 7 Jakub Jelinek 2018-03-19 15:47:24 UTC
Created attachment 43702 [details]
gcc8-pr84945.patch

Untested fix with __cpu_features2 var for features 32+.
Comment 8 Jakub Jelinek 2018-03-20 08:15:20 UTC
Author: jakub
Date: Tue Mar 20 08:14:42 2018
New Revision: 258673

URL: https://gcc.gnu.org/viewcvs?rev=258673&root=gcc&view=rev
Log:
	PR target/84945
	* config/i386/i386.c (fold_builtin_cpu): For features above 31
	use __cpu_features2 variable instead of __cpu_model.__cpu_features[0].
	Use 1U instead of 1.  Formatting fixes.

	* gcc.target/i386/pr84945.c: New test.

	* config/i386/cpuinfo.h (__cpu_features2): Declare.
	* config/i386/cpuinfo.c (__cpu_features2): New variable for
	ifndef SHARED only.
	(set_feature): Define.
	(get_available_features): Use set_feature macro.  Set __cpu_features2
	to the second word of features ifndef SHARED.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr84945.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/i386/cpuinfo.c
    trunk/libgcc/config/i386/cpuinfo.h
Comment 9 David Binderman 2018-03-20 08:45:04 UTC
It might be worthwhile putting in some code to detect when the number
of features goes over 64. Maybe a simple assert would be enough.

I suspect there is no need yet to have code to handle the over 64 case.

Just an idea.
Comment 10 Jakub Jelinek 2018-03-20 08:55:16 UTC
Fixed.
Comment 11 Jakub Jelinek 2018-04-16 11:23:11 UTC
Author: jakub
Date: Mon Apr 16 11:22:40 2018
New Revision: 259398

URL: https://gcc.gnu.org/viewcvs?rev=259398&root=gcc&view=rev
Log:
	PR target/84945
	* config/i386/cpuinfo.c (set_feature): Wrap into do while (0) to avoid
	-Wdangling-else warnings.  Mask shift counts to avoid
	-Wshift-count-negative and -Wshift-count-overflow false positives.

Modified:
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/i386/cpuinfo.c