Initial patch to make gem5 compile with clang/llvm

Information
Submitter:	Koan-Sin Tan
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

clang: Enable compiling gem5 using clang 2.9 and 3.0

This patch adds the necessary flags to the SConstruct and SConscript
files for compiling using clang 2.9 and later (on Ubuntu et al and OSX
XCode 4.2), and also cleans up a bunch of compiler warnings found by
clang. Most of the warnings are related to hidden virtual functions,
comparisons with unsigneds >= 0, and if-statements with empty
bodies. A number of mismatches between struct and class are also
fixed. clang 2.8 is not working as it has problems with class names
that occur in multiple namespaces (e.g. Statistics in
kernel_stats.hh).

clang has a bug (http://llvm.org/bugs/show_bug.cgi?id=7247) which
causes confusion between the container std::set and the function
Packet::set, and this is currently addressed by not including the
entire namespace std, but rather selecting e.g. "using std::vector" in
the appropriate places.

Testing Done

This is awesome!  Thanks for the work.  There are two things I don't like: the namespace moves and the pkt->Packet:: stuff.  I'd like to figure out ways to make it work without that.

Also, did you make sure that this all still compiles with GCC?

Koan-Sin Tan Jan. 11, 2012, 9:45 a.m. (Jan. 11, 2012, 9:45 a.m.)

Thanks, let's start from easy stuff, the pkt->Packet:: stuff. WIthout Packet::, clang complains "lookup of 'set' in member access expression is ambiguous", because of the ambiguity between STL set and Packet::set. I can think of two trivial alternative ways to deal with the problem.
   1. add '-Wno-ambiguous-member-template' to CCFLAGS
   2. get rid of "using namespace std;", which means adding "std::" when necessary
Which one do you prefer? Did I miss any trivial solution?

The namespace move is trick. Yes, it's a hack. Let's use the change in 'src/arch/alpha/isa/main.isa" as an example to illustrate this problem. Without the change, I got error message like:
  In file included from build/ALPHA_FS/arch/alpha/atomic_simple_cpu_exec.cc:17: build/ALPHA_FS/arch/generic/memhelpers.hh:67:15: error: call to function 'gtoh' that
      is neither visible in the template definition nor found by argument-dependent
      lookup
        mem = gtoh(mem);
I know we should use 'using namespace AlphaISA;"  or "using namespace LittleEndianGuest;' earlier so that clang will know 'gtoh()' is the the LittleEndianGuest one. But I don't know where is the right place to add it. Any suggestion? 

And, yes, I compiled changed code with gcc-4.2 (what comes with Xocde 4.2) and gcc-4.6 without problem.

src/arch/alpha/process.cc (Diff revision 1)

Can you explain a little bit about what's going on here that this is necessary?  I'd like to figure out another way to do this that doesn't seem so hackish.

src/arch/x86/intmessage.hh (Diff revision 1)

This is odd.  Any idea what's going on here?

src/base/fast_alloc.cc (Diff revision 1)

Anyone know why this pragma is here in the first place?  Be nice to see a comment if anyone has a clue.  Steve?  Your code :)

Steve Reinhardt Jan. 11, 2012, 3:15 a.m. (Jan. 11, 2012, 3:15 a.m.)

Actually I don't know anything about this, but it's so old it predates our history (goes back to changeset 2!) so I can't determine whether it's because someone else put it in or if I did and just forgot about it.  For my own dignity I will claim the former.

Nathan Binkert Jan. 12, 2012, 2:51 a.m. (Jan. 12, 2012, 2:51 a.m.)

I think that you can just safely remove the #pragma.  It's for a gcc feature that we just don't (and have never) used.

src/cpu/func_unit.hh (Diff revision 1)

Why is this done?  Does clang differentiate between classes and structs?

Koan-Sin Tan Jan. 11, 2012, 10:57 a.m. (Jan. 11, 2012, 10:57 a.m.)

With -Wall on, clang warns about the difference between classes and structs. And I think C++ standard does care the difference, although GCC dones't. Anyway, if you don't like these class/struct changes, I can turn off them by adding  "-Wno-mismatched-tags" to CCFLAGS.

Nathan Binkert Jan. 12, 2012, 2:56 a.m. (Jan. 12, 2012, 2:56 a.m.)

I'm ok with these changes and I slightly prefer to keep classes and structs consistent if that is the proper thing to do.  I see that you ripped them out in your newer diff and turned off the warning with CCFLAGS.  If you'd prefer to just keep that, I'm OK with that too.

src/cpu/o3/inst_queue.hh (Diff revision 1)

Don't comment out code.  Just delete it.

src/dev/copy_engine.cc (Diff revision 1)

I'd like to figure out why this is necessary and make it go away.

src/python/m5/SimObject.py (Diff revision 1)

Dont' comment out code, please remove it.

Change Summary:

1.  remove pkt->Packet:: stuff, by removing 'using namespace std' and adding std:: when necessary
2. get rid of namespace move, by adding 'using namespace xxx' to src/arc/xxx/isa_traits.hh
3. add '-Wno-mismatched-tags' to CCFLAGS so that we can avoid class/struct changes

Diff:

Revision 2 (+60 -60)

Show changes

	SConstruct
	src/SConscript
	src/arch/alpha/isa_traits.hh
	src/arch/alpha/tlb.cc
	src/arch/arm/insts/static_inst.hh
	src/arch/arm/insts/vfp.hh
	src/arch/arm/isa/templates/basic.isa
	src/arch/x86/isa_traits.hh
	20 more

src/arch/x86/bios/intelmp.cc (Diff revision 2)

Should we do this based on if(sizeof(T) > 1), else guestVal = 0?

Koan-Sin Tan Jan. 11, 2012, 11 p.m. (Jan. 11, 2012, 11 p.m.)
```
sounds good
```

src/arch/x86/isa_traits.hh (Diff revision 2)

Opening namespaces in headers is never a good idea. This essentially means every place we include it this namespace is now opened. Is this really what we want?

Koan-Sin Tan Jan. 11, 2012, 11:05 p.m. (Jan. 11, 2012, 11:05 p.m.)

There are some gtoh() in .hh files. I don't know how to get them compiled without making either big changes or ugly 'using namepace ...'  before some "#include xxx.hh" yet

Nathan Binkert Jan. 12, 2012, 2:53 a.m. (Jan. 12, 2012, 2:53 a.m.)

What really needs to happen is that you get the endianness from the ISA.  I believe that you may be able to simply say TheISA::gtoh() and make it work.  Can you try that?

Koan-Sin Tan Jan. 12, 2012, 9:17 a.m. (Jan. 12, 2012, 9:17 a.m.)

Nathan, 
  Yes, adding "TheISA::" works. I'll update the patch later

src/dev/copy_engine.cc (Diff revision 2)

Why does the std cause a problem with the packet->set? Packet does not inherit from any STL container class. I don't get why there is any potential overloading.

Koan-Sin Tan Jan. 11, 2012, 11:06 p.m. (Jan. 11, 2012, 11:06 p.m.)

this kind of ambiguity has nothing to do with inheritance as far as I can tell

Andreas Hansson Jan. 12, 2012, 5:42 p.m. (Jan. 12, 2012, 5:42 p.m.)

Obviously clang thinks there is more than one "set" member function on a Packet, and I do not really see why the STL container set (which I thought was the one causing the problem) could be part of the picture. 

In short, why is there any ambiguity if a packet only has one set member function?

Change Summary:

1. use TheISA:: instead of introducing 'using namespace' in headers, as suggested by Nathan
2. check sizeof(guestVal) before doing "guestVal >>= 8", as suggested by Andreas

Diff:

Revision 3 (+92 -93)

Show changes

	SConstruct
	src/SConscript
	src/arch/alpha/tlb.cc
	src/arch/arm/insts/static_inst.hh
	src/arch/arm/insts/vfp.hh
	src/arch/arm/isa/templates/basic.isa
	src/arch/generic/memhelpers.hh
	src/arch/mips/faults.hh
	23 more

Overall, this looks a lot better.  The only thing I really want an answer on is why you got rid fo the "using namespace std" in those .cc files.  Other than that, this is basically ready.

SConstruct (Diff revision 3)

I'm happy to have you fix the struct/class stuff and get rid of -Wno-mismatched-tags

src/arch/arm/isa/templates/basic.isa (Diff revision 3)

Don't need the comment here.

src/dev/ide_ctrl.cc (Diff revision 3)

This should be ok (it's in a .cc file).  What's wrong?

Ali Saidi Jan. 18, 2012, 1:51 p.m. (Jan. 18, 2012, 1:51 p.m.)

It's the std::set vs Packet::set issue.

It looks like a clang bug... http://llvm.org/bugs/show_bug.cgi?id=7247

Ali

Gabe Black Jan. 18, 2012, 2:27 p.m. (Jan. 18, 2012, 2:27 p.m.)

What about adding "using std::set;"? Or is it "using namespace std::set"? One of those might lead clang to the right answer without adding std:: all over.

Andreas Hansson Jan. 18, 2012, 5:37 p.m. (Jan. 18, 2012, 5:37 p.m.)

Indeed, instead of the "using namespace std", we could add the appropriate "using std::list", "using std::vector" etc. From the clang bug description it looks like it should be resolved?

Nathan Binkert Jan. 18, 2012, 11:45 p.m. (Jan. 18, 2012, 11:45 p.m.)

I think that we're approaching the realm of having talked about this patch way too much, but I like the idea of the using namespace std::list, etc. instead of sprinkling std:: all over.  Mainly because the latter might get undone and break clang, but with the former, we can put in a comment.

Either way, I think that we should have a comment like this up where the using namespace should be.

// Clang doesn't deal with the existence of std::set and Packet::set properly, so we
// can't do this:
// using namespace std;

src/dev/ns_gige.cc (Diff revision 3)

Same here

src/arch/x86/bios/intelmp.cc (Diff revision 3)

Just change T to uint64_t and you won't need the if (sizeof(...

SConstruct (Diff revision 3)

I would imagine we should also add "+ main[CLANG]" in this test.

Koan-Sin Tan Jan. 18, 2012, 7:48 p.m. (Jan. 18, 2012, 7:48 p.m.)
```
sounds right
```

SConstruct (Diff revision 3)

The CheckHeader test fails when I try to build using clang, and the reason seems to be that clang in contrast to gcc cannot find asm/errno.h that is included via Python.h. On a x86_64 machine I had to forcefully add "-I/usr/include/x64_64-linux-gnu" for the build to proceed. Did anyone else have these issues?

Koan-Sin Tan Jan. 18, 2012, 7:48 p.m. (Jan. 18, 2012, 7:48 p.m.)

this is a bit tricky. Simple answer is to try something like 'scons build/ARM_SE/m5.opt CXX=clang++ CC=clang". If this works, I know the reason :-)

Andreas Hansson Jan. 18, 2012, 7:53 p.m. (Jan. 18, 2012, 7:53 p.m.)

tried both export CC=clang; export CXX=clang++ and specifying on the command line as suggested...but no luck.

it seems to be a known issue, at least based on the number of google hits: 
> "asm/errno.h" is in the directory /usr/include/<host triple>. gcc has that directory on its search list, but clang doesn't

Koan-Sin Tan Jan. 18, 2012, 8:06 p.m. (Jan. 18, 2012, 8:06 p.m.)

It looks like it's a configuration problem of your clang. Clang can find asm/error.h (located at /usr/include/arml-linux-gnueabi/) on my Pandaboard running ubuntu 11.10

Ali Saidi Jan. 21, 2012, 3:19 a.m. (Jan. 21, 2012, 3:19 a.m.)

It's some combination of an ubuntu problem (not creating a a symlink for /usr/include/asm) and clang, not understanding you might want to compile for multiple architectures, so having the architecture independent stuff in another directory is better. With ubuntu you can get around the problem by installing gcc-multilib or symlinking usr/include/asm

I think is this good and plan to commit it. I assume you're fine with your name being on the patch?

Thanks for all of your effort.

Change Summary:

1. Andreas Hansson provided informative description for the patch
2. remove -Wno-mismatched-tags and make class/struct consistent
3. some small changes discussed during review

Description:

~		Initial patch to make gem5 compile with clang/llvm. Tested on Mac OS X 10.7.2 + clang/llvm from Xcode 4.2
	~	clang: Enable compiling gem5 using clang 2.9 and 3.0
	+
	+	This patch adds the necessary flags to the SConstruct and SConscript
	+	files for compiling using clang 2.9 and later (on Ubuntu et al and OSX
	+	XCode 4.2), and also cleans up a bunch of compiler warnings found by
	+	clang. Most of the warnings are related to hidden virtual functions,
	+	comparisons with unsigneds >= 0, and if-statements with empty
	+	bodies. A number of mismatches between struct and class are also
	+	fixed. clang 2.8 is not working as it has problems with class names
	+	that occur in multiple namespaces (e.g. Statistics in
	+	kernel_stats.hh).
	+
	+	clang has a bug (http://llvm.org/bugs/show_bug.cgi?id=7247) which
	+	causes confusion between the container std::set and the function
	+	Packet::set, and this is currently addressed by not including the
	+	entire namespace std, but rather selecting e.g. "using std::vector" in
	+	the appropriate places.

Diff:

Revision 4 (+251 -388)

Show changes

	SConstruct
	ext/libelf/SConscript
	src/SConscript
	src/arch/alpha/tlb.hh
	src/arch/alpha/tlb.cc
	src/arch/arm/miscregs.cc
	src/arch/arm/insts/static_inst.hh
	src/arch/arm/insts/vfp.hh
	80 more

Change Summary:

it seems previous diff is not clean, regenerate one

Diff:

Revision 5 (+251 -208)

Show changes

	SConstruct
	ext/libelf/SConscript
	src/SConscript
	src/arch/alpha/tlb.hh
	src/arch/alpha/tlb.cc
	src/arch/arm/miscregs.cc
	src/arch/arm/insts/static_inst.hh
	src/arch/arm/insts/vfp.hh
	79 more

Ship It!

Andreas Hansson Jan. 29, 2012, 5 p.m. (Jan. 29, 2012, 5 p.m.)

Just to clarify, all regressions pass with this patch using both clang and gcc. The reason I wanted it re-posted is mainly two changes:

1) setRequest that is no longer an overloaded virtual function in src/cpu/inorder/resource.hh, the different resources call the setRequest in the base class rather than overloading it

2) takeOver(From) in classes that inherit from SimObject, e.g. src/cpu/o3/iew_impl.hh, they were hiding the takeOverFrom(BaseCPU*) and thus I have changed the name to takeOver in these cases. Overall, I would be very happy to change this name all together, but this is the smallest change that solves the problem.

This is great.  Thanks for the effort!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)