correctcomputation / checkedc-clang

This is the primary development repository for 3C, a tool for automatically converting legacy C code to the Checked C extension of C, which aims to enforce spatial memory safety. This repository is a fork of Checked C's.
14 stars 5 forks source link

Assertion Failure when converting function defined by macros #255

Closed aaronjeline closed 4 years ago

aaronjeline commented 4 years ago
#include <math_checked.h>

#define ADDX(x)\
  int add##x(int* y) {\
    return *y;\
  }

ADDX(2)

int main(void) {
  int x = 3;
  return add2(&x);
}

Fails with:

cconv-standalone: /home/aeline/checkedc-clang/clang/lib/CConv/ProgramInfo.cpp:671: FVConstraint* ProgramInfo::getFuncFVConstraint(clang::FunctionDecl*, clang::ASTContext*): Assertion `!F->hasBody()' failed.

Weirdly, if the math library is not included, this assertion failure doesn't happen. Instead, no changes are made to the file. (Both math.h and math_checked.h cause this issue).

john-h-kastner commented 4 years ago

I tried inlining math.h and all transitively included headers and then reducing the result with cvise. It doesn't tell me anything other than that there's something weird going on here.

https://gist.github.com/jackastner/27a595010924b5729e753aa9eb6d5d2b

john-h-kastner commented 4 years ago

OK, so this has something to do with the size of the file being converted. That's why cvise couldn't reduce it below a couple hundred lines.

I put together a script to test the idea that this issue depends on file size:

require 'parallel'
Parallel.map((0..10000), in_threads: 12) do |num_concats|
  temp_name = `mktemp --suffix=.c`.strip()
  File.open(temp_name, 'w') { |temp_file|
    temp_file.write("#define c(g) void F#{"O"*100}##g () \n") 
    for i in (0..num_concats) do
      temp_file.write("c(BAR#{i});\n")
    end

    temp_file.write("#define u(d) void add##d(g) {}\nu(2);")
  }
  error = `cconv-standalone #{temp_name} 2>&1 > /dev/null`
  if error.include? "!F->hasBody()"
    puts "failed at #{num_concats}"
    `cp #{temp_name} .`
    `rm #{temp_name}`
    exit
  elsif error.include? "Aborted (core dumped)"
    puts "other fail at #{num_concats}"
    puts temp_name
  end
  `rm #{temp_name}`
end

This finds a failing test case after inserting c(BAR) 37 times giving a smaller and simpler test case than the one obtained by reducing math.h.

#define c(g) void FOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO##g () 
c(BAR0);
c(BAR1);
c(BAR2);
c(BAR3);
c(BAR4);
c(BAR5);
c(BAR6);
c(BAR7);
c(BAR8);
c(BAR9);
c(BAR10);
c(BAR11);
c(BAR12);
c(BAR13);
c(BAR14);
c(BAR15);
c(BAR16);
c(BAR17);
c(BAR18);
c(BAR19);
c(BAR20);
c(BAR21);
c(BAR22);
c(BAR23);
c(BAR24);
c(BAR25);
c(BAR26);
c(BAR27);
c(BAR28);
c(BAR29);
c(BAR30);
c(BAR31);
c(BAR32);
c(BAR33);
c(BAR34);
c(BAR35);
c(BAR36);
c(BAR37);
#define u(d) void add##d(g) {}
u(2);
john-h-kastner commented 4 years ago

Looking at the AST dumps for iterations 37 (crashing) and 36 (passing), there's a difference in the spelling location for add2. In the failing case, it is <scratch space>:3:1. In the passing case it is <scratch space>:39:1. <scratch space>:3:1 is also the spelling location for one of the c(BAR) instances, so we have two declarations which appear to be in the same location.

What I think is happening is that the 37 macro concatenation expansions are enough to consume whatever scratch space clang initially allocates. For the next expansion, clang is forced to allocated a new scratch buffer. The line and column number for a source location in this buffer are based on the offset into this new buffer, but the file name used for this buffer is <scratch space> which conflict with any previously allocated buffer.

This means that it is not sufficient to identify a source location by the filename, line, column triple, and our code needs to be updated to reflect this.

john-h-kastner commented 4 years ago

See issue #18