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

[Mehdi Amini via llvm-dev]

> On Feb 9, 2016, at 6:43 PM, Duncan P. N. Exon Smith via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> +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?

I wonder if it isn't it a bit to much detailed to put this in the release notes? I mean we change the C++ API all the time...

Here is the level of details we went with in 3.7: http://llvm.org/releases/3.7.0/docs/ReleaseNotes.html#non-comprehensive-list-of-changes-in-this-release


-- 
Mehdi