Expr::isIntegerConstantExpr() gives bogus results with sizeof()

Clang Frontend
#1

Hi,

Expr::isIntegerConstantExpr() seems to return bogus results with the sizeof() operator.
For example, for input "21-2*2+4", it returns 21 as expected
but with sizeof:

int buf_src[4];
sizeof(buf_src) => 128 (4 * 4 * 8 bits)
sizeof(buf_src)-sizeof(buf_src)/sizeof(*buf_src) => 124 (???)

So it seems that sizeof() is being computed in bits, but then the operations aren't consistent. (in fact it should be computed in bytes, as that's the value of that operator in C/C++)

Proposed patch:

Index: Expr.cpp

#2

Yep, you're right, applied, thanks!

-Chris

#3

Should the fix instead go in the body of getTypeSize() instead of within isConstantExpr()? Should getTypeSize() ever return the number of bits instead of the number of bytes? Does this have something to do with bitfields?

#4

So it seems that sizeof() is being computed in bits, but then the
operations aren't consistent. (in fact it should be computed in bytes,
as that's the value of that operator in C/C++)

Yep, you're right, applied, thanks!

-Chris

Should the fix instead go in the body of getTypeSize() instead of within isConstantExpr()? Should getTypeSize() ever return the number of bits instead of the number of bytes? Does this have something to do with bitfields?

This is the right solution. We want getTypeSize to be unambiguous as to units, and bits are the most constant thing there are. Various strange things like bitfields, non-8-bit-bytes, and datatypes that are not even multiples of byte sizes all conspire to wanting to have a simple and unambiguous unit to represent things in, bits is the right way to go.

sizeof, on the other hand, is determined in units of sizeof(char). To be fully general, it shouldn't divide by 8, it should divide by TargetInfo.getCharSize().

-Chris

#5

So it seems that sizeof() is being computed in bits, but then the
operations aren't consistent. (in fact it should be computed in bytes,
as that's the value of that operator in C/C++)

Yep, you're right, applied, thanks!

-Chris

Should the fix instead go in the body of getTypeSize() instead of within isConstantExpr()? Should getTypeSize() ever return the number of bits instead of the number of bytes? Does this have something to do with bitfields?

This is the right solution. We want getTypeSize to be unambiguous as to units, and bits are the most constant thing there are. Various strange things like bitfields, non-8-bit-bytes, and datatypes that are not even multiples of byte sizes all conspire to wanting to have a simple and unambiguous unit to represent things in, bits is the right way to go.

Okay. Seems like a valid reason to me.

sizeof, on the other hand, is determined in units of sizeof(char). To be fully general, it shouldn't divide by 8, it should divide by TargetInfo.getCharSize().

Should we go ahead and do this?

Nuno: Thanks again for finding this bug.

#6

I am not sure this is the right forum for reporting LLVM build problems. But here it is. After a recent update, I get this build error building llvm:

llvm[2]: Compiling Lexer.cpp for Debug build
/Volumes/sandbox/llvm/lib/AsmParser/Lexer.l: In function ‘int llvmAsmlex()’:
/Volumes/sandbox/llvm/lib/AsmParser/Lexer.l:278: error: ‘PURE’ was not declared in this scope
/Volumes/sandbox/llvm/lib/AsmParser/Lexer.l:279: error: ‘CONST’ was not declared in this scope
Lexer.cpp: In function ‘int yy_get_next_buffer()’:
Lexer.cpp:2543: warning: comparison between signed and unsigned integer expressions
make[2]: *** [/Volumes/sandbox/llvm/lib/AsmParser/Debug/Lexer.o] Error 1
make[1]: *** [AsmParser/.makeall] Error 2
make: *** [all] Error 1

- Fariborz

#7

Remove /Volumes/sandbox/llvm/lib/AsmParser/Lexer.cpp file and redo make.

#8

That worked for me over the weekend, but now there are other build issues. Building a fresh checkout of LLVM now fails on Leopard:

llvm[2]: Linking Debug executable llvm-as
llvm[2]: Compiling GraphPrinters.cpp for Debug build
llvm[2]: Linking Debug executable llvm-dis
llvm[2]: Compiling PrintSCC.cpp for Debug build
llvm[2]: Compiling UpgradeLexer.cpp for Debug build
Undefined symbols:
   "llvm::PointerType::get(llvm::Type const*)", referenced from:
       llvm::BitcodeReader::ParseTypeTable() in libLLVMBitReader.a(BitcodeReader.o)
       llvm::BitcodeReader::ParseFunctionBody(llvm::Function*) in libLLVMBitReader.a(BitcodeReader.o)
       llvm::GetElementPtrInst::GetElementPtrInst<llvm::Value**>(llvm::Value*, llvm::Value**, llvm::Value**, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, llvm::Instruction*)in libLLVMBitReader.a(BitcodeReader.o)
   "llvm::Pass::getPassName() const", referenced from:
       vtable for llvm::PrintModulePassin llvm-dis.o
   "llvm::FreeInst::FreeInst(llvm::Value*, llvm::Instruction*)", referenced from:
       llvm::BitcodeReader::ParseFunctionBody(llvm::Function*) in libLLVMBitReader.a(BitcodeReader.o)
   "vtable for llvm::InvokeInst", referenced from:
...

#9

sizeof, on the other hand, is determined in units of sizeof(char). To be fully general, it shouldn't divide by 8, it should divide by TargetInfo.getCharSize().

Should we go ahead and do this?

Done, thanks,

-Chris