Wiki code for Bug Fixing Notes

Hide Line numbers
1: == Common Mistakes and How to Fix Them ==
2: 
3: Most of the practices listed here are incorrect regardless of the compiler you
4: use; gcc just happens to be very good at catching them and warning you.  In
5: fact gcc’s warnings are more like lint’s than a normal compiler’s.  And of
6: course gcc can catch many errors which lint does not, or has been configured
7: within ON to disregard.
8: 
9: ~----
10: 
11: So you’ve seen all the tonic-gcc bugs, and you’re wondering what’s the deal. Well, here it is: Tonic needs to be able to build with gcc, and our code currently upsets it. Mostly this is just making the code cleaner but gcc also catches a few bugs.
12: 
13: gcc is pretty good about telling you what’s wrong, but here are the main types of problems we ran into:
14: 
15: * Implicit ints
16: 
17: Old code sometimes treats int as optional (or left it off function declarations long before void existed). Such as:
18: 
19: {{{
20:     f(){}
21: }}}
22: 
23: {{{
24:     c.c:15: warning: return type defaults to ’int’
25: }}}
26: 
27: Or do:
28: 
29: {{{
30:     register i;
31: 	
32:     c.c:16: warning: type defaults to ’int’ in declaration of ’i’
33: }}}
34: 
35: These are easy, just add the int in the right place (and delete register, let the compiler optimize, if it even pays attention to that keyword).
36: 
37: * pointer<->int assignments that aren’t the same size
38: 
39: There are a lot of places that assign pointers to ints (or the other way around) where they both aren’t the same size. This can be bad as gcc seems to treat pointers as signed and might sign-extend the pointer:
40: 
41: {{{
42:     {mike_s:reliant:466} cat c.c
43:     #include <stdio.h>
44:     #include <sys/types.h>
45:     #include <stdlib.h>
46:     int
47:     main()
48:     {
49:             uint64_t        i;
50:             uint64_t        \*p;
51:             p = &i;
52:             i = (uint64_t)p;
53:             (void) printf("p = %p\n", (void \*)p);
54:             (void) printf("i = %llx\n", i);
55:     }
56:     {mike_s:yavin:467} cc -o c c.c
57:     {mike_s:yavin:468} ./c
58:     p = ffbfedb8
59:     i = ffbfedb8
60:     {mike_s:yavin:469} gcc -o c c.c
61:     c.c: In function ’main’:
62:     c.c:13: warning: cast from pointer to integer of different size
63:     {mike_s:yavin:470} ./c
64:     p = ffbfeda8
65:     i = ffffffffffbfeda8
66: }}}
67: 
68: This can be bad if the integer is expected to be a valid pointer still.  Also note that lint doesn’t catch this either:
69: 
70: {{{
71:     {mike_s:yavin:488} lint -s c.c
72:     {mike_s:yavin:489} lint -s -errchk=%all c.c
73:     {mike_s:yavin:490}
74: }}}
75: 
76: Though perhaps there is an option, or this is just because it matches the compiler’s behavior and cc doesn’t do this.
77: 
78: The fix for this is to have a uintptr_t cast in between. Or to rely on the compiler promoting a uintptr_t to uint64_t anyway and just cast the pointer to uintptr_t in the test program:
79: 
80: {{{
81:     {mike_s:yavin:537} cc -o c c.c
82:     {mike_s:yavin:538} ./c
83:     p = ffbfedb8
84:     i = ffbfedb8
85:     {mike_s:yavin:539} gcc -o c c.c
86:     {mike_s:yavin:540} ./c
87:     p = ffbfeda8
88:     i = ffbfeda8
89: }}}
90: 
91: * Functions that don’t return something when they should
92: 
93: gcc is pretty good about catching functions that fall off the end without returning something, or mix & match return; and return (something), which is just wrong. Sometimes this is fallout from #1 above, where the function is really void but it’s old and defaults to int because there was no void. Other times it is just a bug. And other times it’s that the code really doesn’t hit the end but gcc doesn’t know that. Here is a simple example:
94: 
95: {{{
96:     {mike_s:yavin:611} cat c.c
97:     #include <stdio.h>
98:     #include <sys/types.h>
99:     #include <stdlib.h>
100:     static void
101:     usage(void)
102:     {
103:             (void) printf("you used me wrong\n");
104:             exit(1);
105:             /\* NOTREACHED \*/
106:     }
107:     int
108:     main()
109:     {
110:             usage();
111:             /\* NOTREACHED \*/
112:     }
113: }}}
114: 
115: {{{
116:     {mike_s:yavin:612} gcc -Wall -o c c.c
117:     c.c: In function ’main’:
118:     c.c:19: warning: control reaches end of non-void function
119: }}}
120: 
121: gcc doesn’t understand NOTREACHED, that’s a lint directive (and actually lint doesn’t seem to understand it well either since it still complains main has no return statement). But you can tell gcc a function doesn’t return with the ~_\//NORETURN attribute attached to a prototype (see {{code}}<sys/ccompile.h>{{/code}}). Add this after the includes:
122: 
123: {{{
124:     static void usage(void) \//~_NORETURN;
125: 	
126: And gcc is happy:
127: }}}
128: 
129: {{{
130:     {mike_s:yavin:619} gcc -Wall -o c c.c
131:     {mike_s:yavin:620} 
132: }}}
133: 
134: Sometimes the right fix can be to add a ’default’ case to a switch statement that doesn’t have one. It is the last thing in the function, and all cases return, but the coder knew that he would never call the function with an argument the switch statement wouldn’t handle. So no default was added and a NOTREACHED was put after the switch. That’s (personally) a bad thing to do because someone else could come along and call it wrong, so it would seem better to have a default (failure) case or perhaps an assert added.
135: 
136: Sadly sometimes the last thing a function calls is something that takes an argument that makes it not return, but gcc can’t be told that. Like a generic error printing routine that takes a ’fatal error call exit’ flag.  I have just added bogus returns in that case but perhaps someone can find a better way.
137: 
138: * Undefined order of operations
139: 
140: gcc has caught a few bugs where the order of operations is undefined.  For example, I have seen some function calls like this:
141: 
142: {{{
143:     f(i, i++);
144: }}}
145: 
146: this causes gcc to reply:
147: 
148: {{{
149:     c.c:16: warning: operation on ’i’ may be undefined
150: }}}
151: 
152: because the order function arguments are evaluated is unspecified by the C standard. It may be left to right, like the programmer expected, or it may be right to left, or neither.  This can also happen when an argument with side effects is passed to a macro that evaluates it twice. Or in assignments like this:
153: 
154: {{{
155:     i = i++;
156: }}}
157: 
158: {{{
159:     c.c:17: warning: operation on ’i’ may be undefined
160: }}}
161: 
162: * Main functions defined as returning void
163: 
164: main doesn’t return void, it returns int, and gcc complains.  So don’t do that :) And don’t forget that it may then want main to return something (or lint may) so if you have an exit at the end that can just be a return.
165: 
166: * Extra tokens after preprocessor directives
167: 
168: This is usually for #else and #endif directives. Don’t do this:
169: 
170: {{{
171:     #ifdef DEBUG
172:     ...
173:     #else DEBUG
174:     ...
175:     #endif DEBUG
176: }}}
177: 
178: do this:
179: 
180: {{{
181:     #ifdef DEBUG
182:     ...
183:     #else /\* DEBUG \*/
184:     ...
185:     #endif /\* DEBUG \*/
186: }}}
187: 
188: * Writing into string constants.
189: 
190: gcc puts puts string constants in read-only memory because, well, they are constant. Unfortunately some code writes into string constants because Studio doesn’t do that by default (see -xstrconst).  Sadly this is not something found by compilation but only by testing, and we know of a few:
191: 
192: ** 6264767 cfgadm expects to write into string constants
193: ** 6264775 lpadmin expects to write into string constants
194: ** 6272155 svcs writes into string constants, can crash as a result
195: 
196: There is a flag that can be used to have gcc make string constants non-constant (-fwritable-strings), and we do (sadly) use it. But note what it says:
197: 
198: {{{
199:     {mike_s:yavin:683} /usr/sfw/bin/gcc -Wall -fwritable-strings -o c c.c
200:     cc1: note: -fwritable-strings is deprecated; see documentation for details
201: }}}
202: 
203: And in gcc 4.0 guess what:
204: 
205: {{{
206:     {mike_s:yavin:684} gcc -Wall -fwritable-strings -o c c.c
207:     cc1: error: unrecognized command line option "-fwritable-strings"
208: }}}
209: 
210: Code that wants to write to constants should just be fixed.  Using -xstrconst in the CFLAGS may also help catch these in testing sooner.  You can use the gcc flag -Wwrite-strings to help you find these bugs, but unfortunately it’s too noisy to be useful all the time; any nontrivial code is likely to trigger a few of these.
211: 
212: * Incorrect format strings
213: 
214: Format strings need to correspond to the types being passed, otherwise  gcc complains.  Common problems are:
215: 
216: ** Using {{code}}%p{{/code}} corresponding to an argument which is not a pointer.
217: ** Using {{code}}%x{{/code}} or {{code}}%lx{{/code}} corresponding to a pointer argument.
218: ** Use of incorrect or unnecessary size specifiers.
219: 
220: Here are some usage guidelines:
221: 
222: ** Use {{code}}%d{{/code}} or {{code}}%x{{/code}} for int, int32_t, and, in the kernel, smaller //signed// integer types.
223: ** Use {{code}}%u{{/code}} or {{code}}%x{{/code}} for unsigned int, uint32_t, and, in the kernel, smaller //unsigned// integer types.
224: ** Use the "l" modifier for long, unsigned long, and (u)intptr_t types.  Additionally, use the "l" modifier in the kernel for size_t, off_t, ssize_t, and other types which are equivalent to long or unsigned long.
225: {{{
226:     // Use {{code}}%p{{/code}} for any pointer or caddr_t type.  Do not cast pointers to any other type for the purpose of display (especially types which may be less than 64 bits long) unless you are absolutely sure it is both correct and necessary.  For example, kernel text addresses on SPARC are guaranteed to fit into 32-bit integers, but such a cast is unlikely to be needed if it is only for display/debugging purposes, and in fact it may even hide bugs.  Note that you may need to case to {{code}}void //{{/code}} to make lint happy; this is fine.
227: }}}
228: 
229: Special note on long long and the "ll" modifier: the SPARC kernel is no longer built 32-bit; therefore you may use "ll" only for those variables which are explicitly declared long long or a type equivalent to long long (the most notable examples are hrtime_t variables).  However, code using int64_t or uint64_t and compiled for both 32-bit and 64-bit must instead use the macros in {{code}}<sys/int_fmtio.h>{{/code}}.
230: 
231: Wrong:
232: 
233: {{{
234:     uint64_t    variable;
235:     void        \*ptr;
236:     cmn_err(CE_NOTE, "oh dear: %llx (%x)", variable, (uint_t)ptr);
237: }}}
238: 
239: Right:
240: 
241: {{{
242:     #include <sys/int_fmtio.h>
243:     uint64_t    variable;
244:     void        \*ptr
245:     cmn_err(CE_NOTE, "oh dear: %" PRIx64 " (%p)", variable, ptr);
246: }}}
247: 
248: It is good practice to use these any time a variable’s size is explicit in its type (uint8_t, int16_t, etc).
249: 
250: Note on cmn_err "%b" format: gcc warns if you include a field width with the "%b" specifier.  This is a bug in gcc that will be fixed in the near future.  To work around it, use the -Wno-format flag (with the compiler wrapper, -_gcc=-Wno-format).
251: 
252: * Useless comparisons
253: 
254: Comparisons which are always true or always false will cause gcc to emit warnings (and, in many cases, lint to emit warnings as well).  Common mistakes are comparing unsigned variables with 0 and comparing variables with constants larger than that variable could contain.  Example of wrong code:
255: 
256: {{{
257:     #define    MAX_LITTLE	1024
258:     #define    MAX_MEDIUM	0x10000
259:     uint8_t	little;
260:     uint16_t	medium;
261:     int8_t	slittle;
262:     ...
263:     if (little < 0 || little > MAX_LITTLE)
264:             return;
265:     if (medium < 0 || medium >= MAX_MEDIUM)
266:             return;
267:     if (slittle >= MAX_LITTLE)
268:             return;
269: }}}
270: 
271: Questions:
272: 
273: * Why doesn’t the tonic team fix these?
274: 
275: There are a few reasons. One is that the resources aren’t there.  Another is that it would seem better for the owners of the code to fix it, if there are any owners around. See 6272018, where the RE determined we could just delete a file instead of fixing it.  Or the owners might know that they need to coordinate changes with other folks. There is also the faint hope that more testing would be performed on the result just to be sure there aren’t any optimization-related bugs or perhaps cases where the code writes into string constants.
276: 
277: The bugs were mostly filed in an attempt to identify the minimum change required, and get the change to the appropriate people, who hopefully could then work in parallel with others.
278: 
279: 
280: * Why don’t we just turn off these warnings?
281: 
282: Well, because it seems better to fix the code rather than slap the compiler. After all, it’s not just gcc that may be upset about it, the Studio compiler could easily decide to do the same thing. And in fact does in many cases if C99 is turned on (we turn it off because there were far, far more warnings in ON with it on.  But it would be really nice to have it on as it requires the use of prototypes and prototypes catch problems).  Another reason is that turning off warnings rather than fixing them is one reason the code so upsets gcc - see all the lint warnings we disable and again the fact C99 is disabled. Taking the easy road before is not paying off now.
OpenSolaris

Top Menu

Wiki code for Bug Fixing Notes

Search

Collectives

Community Group

Project

User Group

Subsites

Community Group tools Pages
