[PATCH] D83082: [Alignment][NFC] Use proper getter to retrieve alignment from ConstantInt and ConstantSDNode

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D83082#2132040 <https://reviews.llvm.org/D83082#2132040>, @sbc100 wrote:

> In D83082#2132017 <https://reviews.llvm.org/D83082#2132017>, @jrtc27 wrote:
>
> > In D83082#2131971 <https://reviews.llvm.org/D83082#2131971>, @sbc100 wrote:
> >
> > > This change seems to have broken one of our tests in emscripten.  I reduced it to this:
> > >
> > >   #include <stdlib.h>
> > >   #include <assert.h>
> > >  
> > >   int main() {
> > >     void * ptr = aligned_alloc(3, 64);
> > >     assert(ptr == NULL);
> > >     return 0;
> > >   }
> > >
> >
> >
> > Yeah, this is testing DR 460's new (and accepted) requirements (previously it was undefined):
> >
> > > If the value of alignment is not a valid alignment supported by the implementation or the value of size is not an integral multiple of alignment the function shall fail by returning a null pointer.
>
>
> Regardless of weather its defined or undefined behaviour I wouldn't expect the compiler to crash though right?  Clearly this is a compiler bug, right?


Oh, definitely, I wasn't intending to suggest otherwise.

>> I feel this should probably be handled specially (ie use align(1) or no alignment) in the Clang frontend? Though I wouldn't like to say how things change when you change it to:
>> 
>>   int align = 3;
>>   void *ptr = aligned_alloc(align, 64);
> 
> Same crash happens with this code.

Sure. My point is that if you sufficiently obfuscate it, Clang might not notice what's going on so even if Clang caught these cases, you might have issues with more complex ones later in the pipeline. Though maybe LLVM passes won't move values later determined to be constants into new align attributes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83082/new/

https://reviews.llvm.org/D83082