request for comments on patch: detecting integer undefined behaviors

Clang Frontend
21

In any case, I don't think that there is any reasonable discussion to be had: it's the wrong thing to do for -ftrapv, as I said before, it breaks an established model.

What established model? Every time -ftrapv is mentioned on the GCC lists, the instructions are not to use it.

-ftrapv is documented in the GCC manual and the GCC man pages. The documentation does not say that an additional function need be linked into the program.

I didn't realize that you were implementing it in a gcc compatible way.

I provided a default handler function that would call abort (after printing a helpful error) along with the original patch. It never got committed (I didn't have commit access back then, other people were committing my patches). Mike said it would go in a clang runtime lib when one existed.

Are you sure that I approved the patch and not mike? Do you have a link?

Mike committed it. You reviewed it and asked that the -ftrapu option be removed. Check the mailing list archives for April 2, 2009. We also discussed it in IRC, where I explained both the implementation and my use of it in some detail (if you log private chats, check around April 2-4, 2009 - I don't keep logs).

Alright, well I don't remember that it was implementing something that was incompatible with -ftrapv.

I'm sorry for the confusion on this, I really thought that I emailed you about it.

-Chris

22

Awesome, thanks Chris. More patches to follow.

John

23

David, can you give some details (or a pointer) about the trap handling interface you have been using?

Our trap interface is below. It needs work but it has been useful in making it possible to print highly informative error messages about integer problems.

Moving forward, it seems to make sense to give clang -trapv (or -fcheckv or whatever) the ability to:

- abort

- call a lightweight trap handler, designed for performance

- call a heavyweight trap handler like ours below, designed for verbosity

John

static unsigned long long undefined_behavior_trap_handler (unsigned int line,
                unsigned int column,
                char *filename,
                char *opstr,
                char *rule,
                signed char ub,
                signed char ltype,
                signed char rtype,
                unsigned long long lvalue,
                unsigned long long rvalue)

24

Hi John,

The trap handler I was using was:

long long __overflow_handler(long long a, long long b, unsigned char op, unsigned char width)

Where a and b were the two operands, op represented the operation, and width gave the number of bits in the original. The low bit in op indicated whether it was a signed or unsigned operation, and then the other 7 bits could be used for identify the type of operation (add, subtract, and multiply were the only ones used, as divide can't overflow).

The sign bit was largely superfluous, as Chris removed the -ftrapu option, which would have caused it to be unset, but it is very important for preventing a certain kind of security vulnerability (in particular, when you do malloc(a * b), it would trap if a*b overflowed), because this kind of wrapping is well defined by the standard, even if it's not always used correctly.

The purpose of this was to make implementing calloc()-style wrappers that were overflow-safe trivial - for example by substituting 0 for the result when it overflowed, so the wrapper would just need to be:

void *safe_malloc(size_t a, size_t b)
{
  size_t s = a * b;
  if (s == 0) { return NULL; }
  return malloc(s);
}

This handler is enough for the CERT AIR stuff to generate a helpful error message when you violate the semantics of the model, for other code to throw an exception, and for LanguageKit to be able to box things that overflow in an object.

I'd like to reinstate support for this handler with an -ftrapv-handler= option, if no one objects, so we default to aborting on overflow if -ftrapv is provided alone, and only call the handler if it is.

Your handler looks better suited to providing detailed error diagnostics for a more general case, so I don't believe that there is any conflict between the two.

David

25

Hi David-

Your handler looks better suited to providing detailed error diagnostics for a more general case, so I don't believe that there is any conflict between the two.

Yes, definitely. Let's just make sure to coordinate on the encoding of "op" and also that our arguments are a superset of yours. Should be easy.

John

26

Hi John,

27

Hi John,

Hi David-

Your handler looks better suited to providing detailed error diagnostics for a more general case, so I don't believe that there is any conflict between the two.

Yes, definitely. Let's just make sure to coordinate on the encoding of "op" and also that our arguments are a superset of yours. Should be easy.

I've just committed some stuff (r114192) that restores the old behaviour of -ftrapv-handler= is specified.

David, please get significant changes like this reviewed before they are committed.

This calls the function named as the argument to the option in case of overflow, with the prototype as described in my last mail. Because the trap handler is now specified by name, I've removed the indirect call via the pointer, so you can just define the handler in the compilation unit, if required, or in a separate library. You can now generate the same behaviour that earlier versions of clang generated with -ftrapv by specifying the function that __overflow_handler pointed to with this option.

Please can you take a look and make sure that it doesn't conflict with anything that you want to do.

Chris: Is there any chance of merging this into the 2.8 branch - it would be nice if 2.8 didn't break stuff that worked with 2.7...

Absolutely not, the schedule is well past accepting new features.

-Chris

28

I've just committed some stuff (r114192) that restores the old behaviour of -ftrapv-handler= is specified.

David, please get significant changes like this reviewed before they are committed.

The change is a couple of dozen lines, most of which is in the option handling stuff, and the rest of it is restoring code that was in trunk for over a year - hardly a 'significant' change - and it restores functionality that we are had in the last two releases.

You're adding a major new feature to the compiler. In addition to discussion of whether or not it is a good thing, it should be decided if it's the right way to solve the problem, and the patch should include documentation. You've skipped all of this. The previous behavior in the last two releases had the exact same problems.

I appreciate that the new approach is much more minimal, and I like the design much better than the old. Please propose some documentation for it. Also, why does the handler return a value instead of aborting? I don't see how this is useful.

Please can you take a look and make sure that it doesn't conflict with anything that you want to do.

Chris: Is there any chance of merging this into the 2.8 branch - it would be nice if 2.8 didn't break stuff that worked with 2.7...

Absolutely not, the schedule is well past accepting new features.

So we're going to have to tell people to compile some files with 2.7 and the rest with 2.8. Thanks, I'm sure that will make it much easier to persuade people to adopt clang.

I don't see how this affects adoption. If they haven't adopted it yet, then a change from 2.7->2.8 won't affect them.

-Chris

29

You're adding a major new feature to the compiler. In addition to discussion of whether or not it is a good thing, it should be decided if it's the right way to solve the problem, and the patch should include documentation. You've skipped all of this. The previous behavior in the last two releases had the exact same problems.

I included a lot of documentation in the original patch, including example of usage, but these were not committed anywhere. I'm happy to add them, if you can point me to where clang options are meant to be documented outside of the code (I only see documentation on warnings in the user manual).

I appreciate that the new approach is much more minimal, and I like the design much better than the old. Please propose some documentation for it. Also, why does the handler return a value instead of aborting? I don't see how this is useful.

In this thread, I have already given two cases where it used:

1) In LanguageKit, we promote the overflowed result to an object. This is useful when implementing Smalltalk, JavaScript, Python, and so on compilers with LLVM.

2) In a calloc()-like function, if the calculation overflows you can substitute 0. malloc() will then return NULL and it's just appears like you don't have enough memory (which is true, although in the more specific case that you don't have enough address space). Or some other marker value can be used, so code outside the handler can check for overflow by checking this marker after each operation.

Another one from the CERT paper:

3) It can be used to implement saturation semantics for arithmetic.

And another:

4) It lets you run tested code in permissive mode, where you use the result of the overflowed operation, but use something like libunwind to dump a stack trace to a log somewhere before continuing. This lets you observe the result of continuing with the overflow (i.e. the check doesn't affect the program's semantics) and check whether anything bad actually happens.

I don't see how this affects adoption. If they haven't adopted it yet, then a change from 2.7->2.8 won't affect them.

Because the major negative comment that is raised when I suggest people adopt clang is that they don't want to be tied in to Apple's compiler. It's much harder to convince them that clang is a community project that Apple is the largest contributor when the official policy is that issues that affect Apple code are show stoppers and patches that break Apple code are summarily reverted rather than having trivial fixes applied, while patches that break non-Apple code do not require any code review before committing (as long as Apple's stuff is fine) and are not considered release-blockers.

When you, as project leader, are willing to intentionally remove a feature, knowing that people are using it (as you said in the commit log where you removed this), just before a release, without discussion, it makes it very difficult to describe clang as a community project.

David

30

Given the -ftrapv-handler option now in the tree, the most straightforward way to support our heavier-weight handler would seem to be something like a new option "-ftrapv-handler-verbose".

Would this be looked upon favorably?

Thanks,

John

31

Is it possible to unify the two somehow?

-Chris

32

Is it possible to unify the two somehow?

Hi Chris-

If you mean "can a single trap handler provide both verbose error messages and also high-performance interposition like David wants" then I would expect that the answer is no. It takes time and bloats code size to marshall up all the arguments that our handler wants.

On the other hand, our interface is basically a superset of David's. My belief is that suporting both trap handling interfaces will not be invasive at all, and that it is useful to support both. The only difference is that when compiling a program in verbose trap mode, the trap-handling BBs will contain more stuff.

John

33

What I mean to say is that the overflow checking logic is the actual invasive part, and this is already unified. The handler that gets called seems like a detail.

John