[llvm-dev] Question about an error we're now starting to get on LLVM 3.8.0rc2since

[Duncan P. N. Exon Smith via llvm-dev]

+llvm-dev

> On 2016-Feb-09, at 12:54, Harris, Kevin <Kevin.Harris at unisys.com> wrote:
> 
> Duncan,
>         Kevin Harris here, from Unisys.  In our application, generating LLVM IR, we have several instances of code that looks like this:
>  
>         . . .
>         Value* pDS;
>         . . .
>             auto argIt = pFunc->arg_begin();
>             pDS = argIt;
>         . . .
>  
>         This construct works fine in 3.7, but in LLVM 3.8.0rc2, I’m now getting an error:
>  
> g++-5.2.0 `/home/kharris/dyntrans/llvm-install/bin/llvm-config --cxxflags` -D JITREV="\"4793\"" -D GPPVERSION="\"5.2.0"\" -D LLVMVERSION=38 -D LLVMBUILDVERSION="\"3.8.0"\" -D LLVMBUILDTYPE="\"Debug+Asserts"\" -fno-rtti -Wall -Wextra -Wshadow -c -fmessage-length=0 -fPIC -std=c++11 -o "Debug/dt_llvm.o" "Debug/../dt_llvm.cpp" -I ../MarinerIP -O3 -g3 -m64 -march=core2 -MMD -MP -MF "Debug/dt_llvm.d" -MT "Debug/dt_llvm.d"
> Invoking: g++-5.2.0 Compiler
>  
> Debug/../dt_llvm.cpp: In static member function âstatic llvm::Function* BREGS::selectBasicBreg(Access_t)â:
> Debug/../dt_llvm.cpp:1772:17: error: cannot convert âllvm::ilist_iterator<llvm::Argument>â to âllvm::Value*â in assignment
>              pDS = argIt;
>                  ^

You probably want `pDS = &*argIt`, which makes the conversion from a
possibly-not-dereferenceable-iterator to a pointer explicit.

>  
>         Evidently, the result of the call to pFunc->arg_begin(); is no longer convertible to a “Value*” type in 3.8.  I checked on the type of an Argument, it is still derived from a Value type, so that isn’t the problem.  But I discovered that an “ArgumentListType” has indeed changed.  In 3.7, in /include/IR/Function.h, this was declared as:
>  
>   typedef iplist<Argument> ArgumentListType;
>  
> whereas in LLVM 3.8.0rc2, this is changed to:
>  
>   typedef SymbolTableList<Argument> ArgumentListType;
>  
>         Rummaging around in the svn log for Function.h, I discovered that this change occurred at svn rev 249602, which you annotated as:
>  
> IR: Create SymbolTableList wrapper around iplist, NFC
>  
> Create `SymbolTableList`, a wrapper around `iplist` for lists that
> automatically manage a symbol table.  This commit reduces a ton of code
> duplication between the six traits classes that were used previously.
>  
> As a drive by, reduce the number of template parameters from 2 to 1 by
> using a SymbolTableListParentType metafunction (I originally had this as
> a separate commit, but it touched most of the same lines so I squashed
> them).
>  
> I'm in the process of trying to remove the UB in `createSentinel()` (see
> the FIXMEs I added for `ilist_embedded_sentinel_traits` and
> `ilist_half_embedded_sentinel_traits`).  My eventual goal is to separate
> the list logic into a base class layer that knows nothing about (and
> isn't templated on) the downcasted nodes -- removing the need to invoke
> UB -- but for now I'm just trying to get a handle on all the current use
> cases (and cleaning things up as I see them).
>  
> Besides these six SymbolTable lists, there are two others that use the
> addNode/removeNode/transferNodes() hooks: the `MachineInstruction` and
> `MachineBasicBlock` lists.  Ideally there'll be a way to factor these
> hooks out of the low-level API entirely, but I'm not quite there yet.
>  
>         Since this change is annotated as NFC, I’m thinking that the actual cause of my error is some other (earlier) change to the way ArgumentListType is derived, but I haven’t found it yet, so I thought I’d bug you first. 

You're looking for r252380.  Relevant excerpt:

    Note: if you have out-of-tree code, it should be fairly easy to revert
    this patch downstream while you update your out-of-tree call sites.
    Note that these conversions are occasionally latent bugs (that may
    happen to "work" now, but only because of getting lucky with UB;
    follow-ups will change your luck).  When they are valid, I suggest using
    `->getIterator()` to go from pointer to iterator, and `&*` to go from
    iterator to pointer.

>  
>         Is this change indeed intended as a visible API change to source code generating references to argument list values?  If so, can you point me to a description of how I should change our code?  Should I bug someone else about this problem?  Should this API change be documented in the 3.8.0 release notes?

Release notes is a good idea; somehow I never think of that.  Hans, is it
too late?