[llvm-commits] [llvm] r78628 - /llvm/trunk/autoconf/configure.ac

[Chris Lattner]

On Aug 11, 2009, at 2:22 PM, Gabor Greif wrote:
>>> URL:http://llvm.org/viewvc/llvm-project?rev=78628&view=rev
>>> Log:
>>> Lay the groundwork for my upcoming ilist sentinel shrinking patch
>>> by defining a LLVM_COMPACT_SENTINELS symbol to 0 or 1 in config.h.
>>
>> Gabor, it is usually a bad idea to make major ABI changes like this
>> based on build settings like ENABLE_OPTIMIZED and DISABLE_ASSERTIONS.
>> Why do you need this?
>
> Hi Chris,
>
> the reason is that I originally had a patch in the tree where
> NDEBUG caused the sentinels to only have a back pointer, but no
> forward pointer.
> Because NDEBUG disabled assertions, no one looked at the forward
> pointer anyway.

But why do you want different behavior based on whether assertions are  
enabled or not?  Again, that's another indicator that major and  
invasive abi changes based on assertions being enable is a problem,  
working around it doesn't make the problem go away. :)

> Now, this turned out to be a suboptimal idea, because llvm-gcc (e.g.)
> sets NDEBUG
> in some rather strange ways, so the clients sometimes were configured
> differently,
> causing major disagreements in the size and field offset of
> llvm::Function, for example.

Right, this is an example of why we don't want abi changes :)

> Now the feature of lean sentinels is an interesting one, since it
> saves 2 pointers per
> llvm::Function and one pointer per llvm::BasicBlock, without any
> additional overhead.

Wow, that would be great.  Can you describe the approach?  Should this  
wait until after 2.6 branches?  I'm somewhat nervous about invasive  
changes like this, and we've had issues in the past with the previous  
ilist shrinking patches.

> This is how I came up with LLVM_COMPACT_SENTINELS in configure. It
> defaults to 1 if
> in Release-Asserts mode. Now if clients include the same config.h and
> link against the
> Release-Asserts libraries, there will be no trouble. I guess this is
> the primary use case.

How about just making it a autoconf switch decoupled from either  
existing setting?  Why exactly don't you want this for non-assert mode?

-Chris