llvm dev - Oct 2014 - [LLVMdev] Large constants in patchpoints

Currently llvm crashes on following code, run through llc:

declare void @llvm.experimental.stackmap(i64, i32, ...)

define void @foo() {
  tail call void (i64, i32, ...)* @llvm.experimental.stackmap(i64 0,
i32 0, i64 9223372036854775807)
  ret void
}

The issue is that 9223372036854775807 (decimal for 0x7fffffffffffffff)
is the "empty key" for an int64_t, and in
StackMaps::recordStackMapOpers we crash when we try to insert this as
a key into ConstPool.  The this happens if we change the constant to
be the tombstone.

Two potential fixes I can think of:

 1. have some special logic to remember the offsets for the tombstone
    and empty i64 constants.  This can easily be tracked using two
    "Optional<int>" fields.

 2. change ConstantPool to be use a std::map instead of a
    llvm::DenseMap as the map in the MapVector.

An aside: the same function has this check "((I->Offset +
(int64_t(1)<<31)) >> 32)" -- won't this cause a signed
overflow (and
hence UB) if I->Offset is negative?

-- Sanjoy

Apologies, please ignore the aside:
> An aside: the same function has this check "((I->Offset +
> (int64_t(1)<<31)) >> 32)" -- won't this cause a signed
overflow (and
> hence UB) if I->Offset is negative?
The overflow won't be a *signed overflow*, so we're okay.

-- Sanjoy

We also could make the DenseMap just an uint64_t. In this case the empty key and
the tombstone would be never encoded into the constant pool, because they can be
encoded in the offset field itself.
> On Oct 31, 2014, at 11:20 AM, Sanjoy Das <sanjoy at
playingwithpointers.com> wrote:
> 
> Currently llvm crashes on following code, run through llc:
> 
> declare void @llvm.experimental.stackmap(i64, i32, ...)
> 
> define void @foo() {
>  tail call void (i64, i32, ...)* @llvm.experimental.stackmap(i64 0,
> i32 0, i64 9223372036854775807)
>  ret void
> }
> 
> The issue is that 9223372036854775807 (decimal for 0x7fffffffffffffff)
> is the "empty key" for an int64_t, and in
> StackMaps::recordStackMapOpers we crash when we try to insert this as
> a key into ConstPool.  The this happens if we change the constant to
> be the tombstone.
> 
> Two potential fixes I can think of:
> 
> 1. have some special logic to remember the offsets for the tombstone
>    and empty i64 constants.  This can easily be tracked using two
>    "Optional<int>" fields.
> 
> 2. change ConstantPool to be use a std::map instead of a
>    llvm::DenseMap as the map in the MapVector.
> 
> An aside: the same function has this check "((I->Offset +
> (int64_t(1)<<31)) >> 32)" -- won't this cause a signed
overflow (and
> hence UB) if I->Offset is negative?
> 
I guess "!isInt<32>(I->Offset)” might be a more readable solution.
> -- Sanjoy

> We also could make the DenseMap just an uint64_t. In this case the empty
key and the tombstone would be never encoded into the constant pool, because
they can be encoded in the offset field itself.
That's a great idea, I'll send in a patch soon.

-- Sanjoy