Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Fix Crashes when 'AttributeList::get'ing with an ArrayRef<AttributeList> where all pImpl are null

Authored by thewilsonator on May 28 2017, 12:44 AM.



As the Title says, this causes crashes if you go

AttributeList::get(getContext(), { AttributeList(),AttributeList() });

It is sensitive to I/O, I have no idea why it is I/O sensitive but it is, at least it disappears when logging is turned on in LDC (the LLVM D Compiler) without fail.
Sometimes the last function in the stacktrace is

_vsnprintf + 586
llvm::AttributeList::get(llvm::LLVMContext&, llvm::ArrayRef<llvm::AttributeList>) + 798

Anyway this appears to fix this and will speed up merges of null AttributeList.


Diff Detail


Event Timeline

thewilsonator created this revision.May 28 2017, 12:44 AM

Hi Nicholas:

This looks like a manifestation of a problem that is possibly elsewhere (quick look makes me think in the getImpl when it is supplied an empty AttrSet). But I will have a better look later and get back. By the way, the context is missing (probably your diff to create the patch didnt have -U9999).


I just pasted the diff from my terminal and it accepted it with whatever the default of git diff is, I don't know what U9999 is. *shrug*
This is my first contribution to LLVM so I don't know the way things are supposed to work.

The AttributeList::get that takes an ArrayRef<AttributeList> checks for Attrs.empty() and for Attrs.size() == 1 as fast paths.
the getImpl asserts that the ArrayRef<AttributeSet> is not empty but I must have had asserts disabled.


No problem. Please have a look at (sec. Requesting a review...). Basically, 'git diff -U9999 your-branch' will create a patch with more context.

rnk edited edge metadata.May 31 2017, 7:16 AM

Thanks for the diagnosis and patch! I think I didn't find this problem when testing LLVM and clang because they test each AttributeList for emptiness before adding it to the list of lists to merge. I'll land this soon.

This revision was automatically updated to reflect the committed changes.