[PATCH] D121063: [AST] Make the last element in the linked list null

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D121063#3364833 <https://reviews.llvm.org/D121063#3364833>, @void wrote:

> In D121063#3364815 <https://reviews.llvm.org/D121063#3364815>, @erichkeane wrote:
>
>> In D121063#3364810 <https://reviews.llvm.org/D121063#3364810>, @void wrote:
>>
>>> In D121063#3364780 <https://reviews.llvm.org/D121063#3364780>, @void wrote:
>>>
>>>> In D121063#3363852 <https://reviews.llvm.org/D121063#3363852>, @erichkeane wrote:
>>>>
>>>>> I suspect this works because we never really treated this as a LL, just as a pair of iterators.  Two things:
>>>>>
>>>>> 1- Can you produce some situation where this is valuable to do?
>>>>
>>>> Yes. In the randstruct feature I'm working on, if this code isn't there it goes into an infinite loop: https://reviews.llvm.org/D120857. It's possible that the way it's constructing and using the list of Decls is somehow wrong, but I wasn't able to identify how.
>>>>
>>>>> 2- Can you switch this over so that the NextInContextAndBits initializes to nullptr/0 so that this line isn't necessary?  I can't imagine we ever re-call this on a decl and have the answer be different.
>>>>
>>>> I'll give it a shot. I'm with @urnathan though that it should have already been like that. :-) Probably just an oversight.
>>>
>>> After looking at the randstruct code again, it's possible that it's not doing the correct thing. (#include "MildShockMeme.h"!) The Decls already have a their pointers set, but then we shuffle them and all Hell breaks loose when we call that function because the end is pointing back to somewhere within the structure beginning. This patch is probably not a bad idea in general, but if you want me to I can fix it on my end.
>>
>> Ah!  Shuffling declarations in a chain is not likely to do 'good things', I'm surprised that this is the first issue we've seen!.
>
> Same :-)
>
>> I presume that there needs to be a 'RebuildDeclChain' for the purposes of 'RandStruct' that first nulls-out the next-in-context-and-bits.
>
> Without this patch, then yes. It might actually be a better idea to have it in the DeclContext anyway just in case someone else wants to use it.
>
>> Alternatively, perhaps "RandStruct" should be 'randomizing' on the first call to this BuildDeclChain function.
>
> I assume that `BuildDeclChain` is called once (and only once) per Record? Will randomizing when calling `BuildDeclChain` mess up the ABI somehow? Or is it safer to do it here because the ABI is decided afterwards?

Interestingly, the ONLY call I see to this is via calls of `LoadLexicalDeclsFromExternalStorage` and `RecordDecl::LoadFieldsFromexternalStorage`.  I suspect that the purpose of this is only for restoring the declaration list after loading a module of some sort.  I see this is the origin of the record-fields one: https://github.com/llvm/llvm-project/commit/0e88a565c0978bb6fd835a33e8069135661a1400 which seems to be most of this function as well.

So I don't have a good idea actually of when this would happen.  It does NOT seem like something that happens during normal compilation I would expect though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121063