Lots of headers don't compile clean with gcc 4.5.3 with -Wshadow

[llvmbot]

Bugzilla Link	11256
Version	trunk
OS	Linux
Reporter	LLVM Bugzilla Contributor

Extended Description

On our project, one of the aspects that we enjoy about LLVM is the fact that it compiles cleanly with –Wall with gcc. We were previously running with a gcc 4.1.x compiler, but we recently upgraded to gcc/g++ 4.5.3. We were glad to see that LLVM 2.9 still compiles cleanly with this version, even though the gnu folks have added several warnings in the intervening time.

Just this week, I decided to try the latest LLVM – the current trunk version (3.1svn) and I also tried the tags/RELEASE_30 version. I was surprised and disappointed to see that, combined with the new gcc, and the changes to the headers in the intervening time, our code no longer compiles cleanly. In particular, we are suffering a spate of “-Wshadow” warnings in the new versions of the headers. See a discussion of this issue at: http://stackoverflow.com/questions/2958457/gcc-wshadow-is-too-strict for some simple examples of this warning.

Whether gcc’s –Wshadow is too strict or not, I found I was able to suppress all these warnings by the simple expedient of changing the spelling of the variables (mostly parameters) mentioned in these warnings. I just added a “1” suffix to each name that appears in a warning, and all uses of the name in any code bodies that appear in the headers.

Since we haven’t systematically attempted to use all the LLVM and Clang headers, we obviously don’t have a complete list of the instances of this issue, but I’ll provide a list of the headers, line numbers associated with the warnings, and names associated with the warnings, for which we’re seeing this problem:

Header                                  line number(s)      variable name(s)

llvm/GlobalValue.h                      62                  Name
llvm/Instructions.h                     611,1265,1274,2739  SubclassData, Value
llvm/ADT/StringRef.h                    72                  data
llvm/ADT/ilist_node.h                   81, 92              Next
llvm/ADT/ArrayRef.h                     57, 61              data, begin, end
llvm/ADT/FoldingSet.h                   481                 Context
llvm/ADT/ValueMap.h                     95, 203, 278, 326   Data, Map, I
llvm/Analysis/CFGPrinter.h              29                  isSimple
llvm/CodeGen/MachineInstr.h             365                 isKill
llvm/ExecutionEngine/ExecutionEngine.h  531                 M
llvm/MC/MCDisassembler.h                58                  STI
llvm/Support/CFG.h                      103                 idx
llvm/Support/TimeValue.h                89,266,315,323,333  seconds, microseconds, milliseconds
llvm/Support/GraphWriter.h              65                  O
llvm/Support/ValueHandle.h              284                 VP
llvm/Support/IRBuilder.h                138                 InsertPoint

In all cases except one, the files are identical between the 3.0 rc1 and the 3.1svn versions (rev 143124). The exception is Instructions.h, where there are 7 additional lines added in the 3.1svn version, prior to the last instance of the problem.

I'd be glad to attach my edited versions of these files upon request.

There are at least 2 open problem reports against -Wshadow behavior in Clang, but this is not related - it is strictly an LLVM header compilation issue.

It would be great if these name changes could be included in the final release version of 3.0. Thanks in advance,
-Kevin

[llvmbot]

These don't occur when building LLVM itself, right?

[llvmbot]

I saw no errors when I did the build, but I don't look at the actual compile commands or the makefile to see what options are used. I am compiling LLVM with the same gcc 4.5.3 compiler, but I don't know if the makefile specifies ordinarily specifies "-Wall".
-Kevin

[llvmbot]

I saw no errors when I did the build, but I don't look at the actual compile commands or the makefile to see what options are used. I am compiling LLVM with the same gcc 4.5.3 compiler, but I don't know if the makefile specifies ordinarily specifies "-Wall".
-Kevin

[llvmbot]

Yes, LLVM is built with -Wall. It is even built with -Wextra (using the old -W
form of this option).

So you see the difficulty: LLVM developers have no way of knowing that this
problem exists, or verifying that the problem is fixed, or checking that the
problem hasn't come back again, since it only impacts your project. I think
the best thing is for you to propose a patch.

[llvmbot]

Thanks for the prompt attention to this!

Yes, I understand the issue. I just double checked our project makefile, and I see that we're also using -Wextra, and that we're using -Wshadow explicitly as well, which I hadn't noticed previously. From this whole episode, can I assume -Wshadow is NOT in the normal LLVM lib compile commands?

Assuming that is true, and the fact that I'm now seeing -Wshadow errors, the question becomes: Why was Release 2.9 -Wshadow clean and pre-3.0 NOT -Wshadow clean? If you give me a hint where to look for the compile command in the makefile(s), I'll hunt around to see if I can figure this out when my current build finishes.

I'm not afraid of proposing a patch (covering all instances of the problem) if it turns out that only I can reproduce it. But if you don't normally use -Wshadow in your builds, with a recent gcc, could I get you to give it a try first and let me know how it goes for you?

[llvmbot]

LLVM is not built with -Wshadow. That's probably because -Wshadow is too
annoying to be useful. Consider the first two warnings I get from gcc-4.6
when building LLVM with -Wshadow:

llvm/include/llvm/ADT/ArrayRef.h: In constructor 'llvm::ArrayRef::ArrayRef(const T*, size_t)':
llvm/include/llvm/ADT/ArrayRef.h:57:7: warning: declaration of 'data' shadows a member of 'this' [-Wshadow]
llvm/include/llvm/ADT/ArrayRef.h: In constructor 'llvm::ArrayRef::ArrayRef(const T*, const T*)':
llvm/include/llvm/ADT/ArrayRef.h:61:7: warning: declaration of 'end' shadows a member of 'this' [-Wshadow]

The lines in question are:

/// Construct an ArrayRef from a pointer and length.
/*implicit*/ ArrayRef(const T *data, size_t length)
  : Data(data), Length(length) {}

/// Construct an ArrayRef from a range.
ArrayRef(const T *begin, const T *end)
  : Data(begin), Length(end - begin) {}

The class also has methods called data and end:

const T *data() const { return Data; }

iterator end() const { return Data + Length; }

This is presumably why gcc is warning. However the code is perfectly clear and
easy to read, and also correct. I doubt the LLVM community will accept patches
renaming data to data1 or whatever.

[llvmbot]

Ahh, so you CAN reproduce it! Well, at least I'm not feeling so alone now. Yes, that's exactly what I'm seeing.

That leaves the issue of why V2.8 and V2.9 were -Wshadow clean but V3.0 and V3.1 are not. I checked our makefile history: we've been using -Wshadow since at least mid-February 2011, with both LLVM 2.8 and 2.9 without incurring these messages. So someone in the LLVM community must have been careful about this until the V3.0 timeframe.

I would normally agree with you that -Wshadow is too picky, but it does occasionally catch real errors (as the discussion in "stackoverflow" shows) and the fixes are indeed trivial. So if it isn't TOO painful to perform the update, I'd still like to move ahead with it.

It would at least be interesting to discover how many instances of this are really out there. Making trivial updates to a couple dozen files seems pretty small a price to pay for allowing a whole new class of bugs to be discovered at compile time. But if the number of instances is really much larger (hundreds?), I can see why you might want to hold off, at least for V3.0.

Did you do a whole build with -Wshadow? If so, how many instances did you see? If not, could you give me a clue what makefile to modify, and how to capture the build messsages, and I'll try to scope the problem myself.

[llvmbot]

Duncan,
Is your commit 143468 related to this issue?
-Kevin

[llvmbot]

Daniel Dunbar did commit 143468, not me.

[llvmbot]

I've stumbled across two additional instances of this problem:

llvm/Analysis/RegionInfo.h  89                   isSubRegion
llvm/Support/CallSite.h     125,209,217,225,233  I,doesNotAccessMemory,
                                                 onlyReadsMemory,doesNotReturn
                                                 doesNotThrow

 -Kevin

[llvmbot]

Compressed version of the -Wshadow warning lines from a V3.0 LLVM+Clang release build

[llvmbot]

After making fixes for all the -Wshadow warnings described previously, for our build using the final release V3.0 headers, and getting a clean build, I finally got 'round to doing a test LLVM+Clang build using -Wshadow.

I generated the warnings by building with gcc 4.5.3, as before, and including "CXXFLAGS=-Wshadow" on the "gmake" command at build time. I then filtered out all the lines with "shadow" in them (the main error messages with the file name and line number of the offending file). Then I ran them through "sort" with the -u option to eliminate all the duplicates. The resulting file has 4874 unique warnings, with 888 coming from LLVM proper, and the rest coming from the Clang portion of the build.

As earlier, almost all of these are from implementations of class methods inside headers, where method arguments (or locals) shadow data members from the same class or a base class. A lot of the Clang warnings are from shadowing instances inside the .cpp files proper (with similar usage).

Since I've already fixed the ones reported earlier, these don't show up in the attached file.

I've attached the resulting file, compressed, and will be happy to provide the raw versions if desired, but anyone should be able to reproduce these as well.

If you try it, let me know if you get results substantially different from these.

Thanks!