-ftrapv

Clang Frontend
1

I've attached a diff which adds -ftrapv to clang-cc (not to clang; the driver code scares me) which uses the new LLVM overflow-checked operations for +, - and * (not ++ or -- yet). When an overflow is detected, it calls a function in a supporting library to handle it. I've provided a default implementation in overflowlib.c. This can be replaced in compiled code by some other function if required. For 100% GCC compatibility just add:

__overflow_handler = abort;

somewhere before the operations that may overflow.

The overflow.c file provided here shows a simple case where an overflow occurs (the volatiles are just there to make absolutely sure no optimisation passes decide to calculate the result at compile time). When you run this (linked against overflowlib.c), you get:

$ ./a.out
Integer overflow! Op unsigned add on 32-bit values, Aborting!
Abort trap: 6 (core dumped)

David

clang.diff (6.94 KB)

overflowlib.c (483 Bytes)

overflow.c (254 Bytes)

2

I've attached a diff which adds -ftrapv to clang-cc

Delicious! Added in r68221.

(not to clang; the driver code scares me)

Check out the code I added to add the option to clang, turns out is is magically easy (for clang_f_options anyway).

When an overflow is detected, it calls a function in a supporting library to handle it. I've provided a default implementation in overflowlib.c.

Hum, I don't know quite what to do with runtime code yet. Logically we need a runtime library. I kinda like the idea of bundling in a reference to the required runtime into the output file, and then having the linker use that knowledge to ensure that the proper runtime is linked in. Conceptually, it would be the same as actually putting the runtime into the output, with link-once style linkage, but cheaper, as one wouldn't need to generate or move all the bits, essentially just the name. gcc would have done this 15 years ago if the linkers supported it, but they didn't. Modern linkers probably could be encouraged to support this for us, if we try.

Absent that, either we need a libclang.{so,dylib,a} or some other suitably advanced technology. Do we have such yet?

This can be replaced in compiled code by some other function if required. For 100% GCC compatibility just add:

__overflow_handler = abort;

I put this in without considering the finer point of exactly how we want this to be. I think it is an excellent start. I do encourage people to weigh in on how they think this should work, if they want to see something else. My quick first impression is I like it exactly how you did it.

3

I've attached a diff which adds -ftrapv to clang-cc

Delicious! Added in r68221.

Thanks

(not to clang; the driver code scares me)

Check out the code I added to add the option to clang, turns out is is magically easy (for clang_f_options anyway).

Wow, that is magically easy. I had it almost right, but the errors I was getting were just confusing me more.

When an overflow is detected, it calls a function in a supporting library to handle it. I've provided a default implementation in overflowlib.c.

Hum, I don't know quite what to do with runtime code yet. Logically we need a runtime library. I kinda like the idea of bundling in a reference to the required runtime into the output file, and then having the linker use that knowledge to ensure that the proper runtime is linked in. Conceptually, it would be the same as actually putting the runtime into the output, with link-once style linkage, but cheaper, as one wouldn't need to generate or move all the bits, essentially just the name. gcc would have done this 15 years ago if the linkers supported it, but they didn't. Modern linkers probably could be encouraged to support this for us, if we try.

I was wondering if we could make the __overflow_hander link-once and set it to abort by default, but allow a linked runtime library to provide another default version of it. Then I realised I actually had no idea about nontrivial linker options and decided to leave this bit for someone who actually knew what they were talking about...

In a future version, I'd quite like to be able to specify the handler function on a per-module basis, so we can have different, statically-defined, overflow behaviour for different modules and allow LLVM to inline the handlers where appropriate.

Absent that, either we need a libclang.{so,dylib,a} or some other suitably advanced technology. Do we have such yet?

This can be replaced in compiled code by some other function if required. For 100% GCC compatibility just add:

__overflow_handler = abort;

I put this in without considering the finer point of exactly how we want this to be. I think it is an excellent start. I do encourage people to weigh in on how they think this should work, if they want to see something else. My quick first impression is I like it exactly how you did it.

I really just wanted something to play with for prototyping the (not-yet-finalised) C1x extensions for overflow checking. My aim was to provide a hook that is sufficiently general to allow experimentation and eventually provide an implementation in libc.so (or libclang.bc) that implements the C1x semantics when they are properly defined.

The other use I have for this is in the Étoilé Smalltalk compiler, which implements messages sent to SmallInt objects in C, and compiles them to bitcode with clang, then links them with the bitcode from Smalltalk classes so that they can be inlined where appropriate. This will let me remove a load of inefficient overflow checking code from that file.

David

4

Missed a bit. The overflow-aware code is called on += and so on, but didn't know about these cases. This patch fixes it.

I'm now using this with Smalltalk for testing for integer overflow in small integers and promoting them to real objects automatically. It seems to be working perfectly, in a lot less code and with better accuracy than the ad-hoc code it replaces.

David

clang.diff (1.47 KB)

5

Missed a bit. The overflow-aware code is called on += and so on, but didn't know about these cases. This patch fixes it.

Checked in r68267.

I'm now using this

You will need to use -ftrapu for SmallTalk. -ftrapv is for signed values only.

6

FYI, it is completely inappropriate for Clang to support a -ftrapu option for SmallTalk. Please remove this.

-Chris

7

I'm not sure I understand this comment. Smalltalk uses signed integers, so -ftrapv, not -ftrapu, is appropriate, but -ftrapu can be useful in a number of cases, for example in the implementation of calloc() which needs to multiply two size_t quantities together and check for overflow, and in any other situation involving computation of array offsets where some additional checks while debugging might be helpful.

What makes the feature inappropriate? Some rationale, beyond 'FYI' would be helpful.

David

8

Checking overflow on a particular multiply two unsigned integers might
be appropriate, but it's not appropriate to change all unsigned
multiplies to check for overflow. The result is well-defined, so any
check would break valid code.

-Eli

9

I agree, although I'd qualify that by pointing out that not all existing code is valid in cases of overflow, and if code is not written with overflows in mind then turning on overflow checking while debugging (although, probably not for release) can help pinpoint bugs caused by unhandled overflows. The calloc() example is the one that immediately came to mind, but there may be others.

Eventually, I'd like to extend this to support different handler functions for different modules, so that you can use it in two different libraries, wanting different overflow checks, without them interfering when linked against the same code. It might also be useful to be able to specify it as an __attribute__ on function, allowing some finer granularity for turning checks on and off.

Obviously, overflow checking should not be the default behaviour for C89/90/99.

David

10

I'm sorry I was too terse. I don't want clang IR generation supporting language features that are not useful for C/C++ etc. Previously we had run-ins where you were trying to adapt the objc runtime generation code to work with your objective-smalltalk compiler, and this was causing the code to get contorted and be slow.

I don't think it is ever a good idea to turn random unsigned multiplies into overflow checked ones, so I don't think that -ftrapu is useful for C programmers, so I think it should be removed.

I *would* be supportive of an attribute on integer types that let programmers "opt in" to overflow checking on particular values. This would be incredibly cool and generally useful because it doesn't break the semantics of C. I just am opposed to a global option that changes how C works.

-Chris

11

I don't think it is ever a good idea to turn random unsigned multiplies into overflow checked ones, so I don't think that -ftrapu is useful for C programmers, so I think it should be removed.

I'm not using -ftrapu, so I don't have any personal problems with it being removed, although, as I said, I can imagine some (hypothetical) cases where it would be useful for debugging.

I *would* be supportive of an attribute on integer types that let programmers "opt in" to overflow checking on particular values. This would be incredibly cool and generally useful because it doesn't break the semantics of C. I just am opposed to a global option that changes how C works.

I definitely agree. The existing code is designed with exactly this in mind. Replacing all potentially-overflowing operations was simply the easiest way of testing it (my initial version of -ftrapv checked signed and unsigned values, Mike split the checks into -ftrapv for signed and -ftrapu for unsigned). I hope to progressively make this finer-grained, but per-file seemed like a good first step.

David

12

Done in r68330.