bugSimulavr: an AVR simulator - Bugs: bug #34270, current master does no build

 
 

bug #34270: current master does no build

Submitter:  Yann Dirson <ydirson>
Submitted:  Sun 11 Sep 2011 08:42:09 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Component Version:  * simulavrxx
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 20 May 2013 02:57:57 PM UTC, comment #9: 

I could run actual sources with swig 2.0.8 and also 1.x.x versions on older systems. I think the bugs are already fixed.

If there are still problems with building wrappers, please reopen this entry or add a new one.



Klaus Rudolph <zfrdh>
Group administrator
Sat 19 Nov 2011 02:50:48 PM UTC, comment #8: 

What version of SWIG are you using? (I use SWIG 2.0.4)

HWStack::m_ThreadList can be changed by its methods (or accessing its members) but cannot be assigned to (overwritten). I think it does not make sense to assign to it, it keeps state of detection of a switch, which is difficult to do right. It might make sense to replace the list of threads in ThreadList::m_threads, though.

A public read-only member looks fine to me, better than having a getter just to accomplish the read-only-ness.

I added diagnostics and asked SWIG to not offer to assign to it:
http://git.savannah.gnu.org/cgit/simulavr.git/commit/?id=2e1ff3d8ebb69a00bedbefcf59ec3829d64c608b

Does it solve the problem?

Petr Hluzin <petrh>
Group Member
Sat 19 Nov 2011 01:50:19 PM UTC, comment #7: 

SWIG does generate code from python/pysimulavr.i. There hwstack.h is included. I do not know if that should happen or not.

But you can make SWIG not to generate a setter function by adding %immutable HWStack::m_ThreadList before the %include.

But it seems strange to me that m_ThreadList is a public member of HWStack if you cannot change it.

Anonymous
Mon 26 Sep 2011 05:58:37 PM UTC, comment #6: 

Ok, using reference this way is a well-formed code according to the standard. Just having read lots of code I can't recall to come across this one. Now I see the fix for it in fact was commited recently.

Marek Pietrzak <mpie>
Group Member
Sun 25 Sep 2011 09:03:41 PM UTC, comment #5: 

Yann Dirson:

It seems that SWIG is generating code (pysimulavr_wrap.cpp, _wrap_HWStack_m_ThreadList_set()) which invokes assignment operator of ThreadList.
It should not try to do the assignment.
In fact hwstack.h is not even in simulavr.i file.

What version of SWIG are you using? (I use SWIG 2.0.4)
If you do need neither Python nor TCL interface you can compile without SWIG.
Or you can try to find why it is generating bindings for the class.

(The error about mutable reference has been now fixed.)


Marek:

The m_core reference is never NULL, never changes, is valid for durationof existence of the ThreadList object.
What guidelines for references do you use?


Everyone:

Where should we add the "#include <cstdio>"?
In every file?
Into global.h and make sure it is included everywhere?
It is pity that GCC does not come with definition of NULL constant.

Petr Hluzin <petrh>
Group Member
Mon 12 Sep 2011 09:26:05 PM UTC, comment #4: 

Great, I can confirm this patch allows the simulator to build with g++-4.4 and 4.5 (and runs, to the minimal extent I have tested), thanks Marek!

Some examples, however, will require some patching as well for use with avr-gcc 4.5, but it's less of an immediate problem for me:

avr-gcc -mmcu=atmega128 -I. -DF_CPU=4000000UL -Os -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums -Wall -Wstrict-prototypes  -c -o StdDefs.o StdDefs.c
In file included from kb.c:14:0:
scancodes.h:19:1: warning: missing braces around initializer
scancodes.h:19:1: warning: (near initialization for ‘unshifted[0]’)
scancodes.h:91:1: warning: missing braces around initializer
scancodes.h:91:1: warning: (near initialization for ‘shifted[0]’)
StdDefs.c: In function ‘putstr’:
StdDefs.c:120:4: warning: pointer targets in passing argument 1 of ‘strlen’ differ in signedness
/usr/lib/gcc/avr/4.5.3/../../../avr/include/string.h:133:15: note: expected ‘const char ’ but argument is of type ‘CHARU
In file included from StdDefs.c:8:0:
/usr/lib/gcc/avr/4.5.3/../../../avr/include/util/delay.h: In function ‘msleep’:
/usr/lib/gcc/avr/4.5.3/../../../avr/include/util/delay.h:153:28: error: __builtin_avr_delay_cycles expects an integer constant.
make[2]: * [StdDefs.o] Error 1
make[2]: * Waiting for unfinished jobs....
make[2]: Leaving directory `/work/yann/elec/avr/simulavr/examples/atmel_key'

That problem is strange, preprocessed code looks fine, not sure what bad interaction we see here (_delay_ms does work, as everybody knows :)

 uint32_t __ticks_dc;
 extern void __builtin_avr_delay_cycles(unsigned long);
 __tmp = ((4000000UL) / 1e3) * __ms;
# 150 "/usr/lib/gcc/avr/4.5.3/../../../avr/include/util/delay.h" 3
  __ticks_dc = (uint32_t)(ceil(fabs(__tmp)));


For g++-4.6, an issue was raised in http://lists.gnu.org/archive/html/simulavr-devel/2011-05/msg00007.html, and CXXFLAGS=-fpermissive does allow to work around the problem.

Yann Dirson <ydirson>
Mon 12 Sep 2011 08:09:06 PM UTC, comment #3: 

All right, give it a try:

diff --git a/src/hwstack.cpp b/src/hwstack.cpp
index eddd686..6c87734 100644
--- a/src/hwstack.cpp
+++ b/src/hwstack.cpp
@@ -28,11 +28,12 @@
 #include "avrmalloc.h"
 #include "flash.h"
 #include <assert.h>
+#include <stdio.h>
 
 using namespace std;
 
 HWStack::HWStack(AvrDevice *c):
-    m_ThreadList(*c),
+    m_ThreadList(c),
     core(c) {
     Reset();
 }
@@ -233,7 +234,7 @@ unsigned long ThreeLevelStack::PopAddr() {
     return val;
 }
 
-ThreadList::ThreadList(AvrDevice & core)
+ThreadList::ThreadList(AvrDevice* core)
  : m_core(core)
 {
  m_phase_of_switch = eNormal;
@@ -262,12 +263,12 @@ void ThreadList::OnReset()
 
 void ThreadList::OnCall()
 {
- m_on_call_sp = m_core.stack->GetStackPointer();
+ m_on_call_sp = m_core->stack->GetStackPointer();
  assert(m_on_call_sp != 0x0000);
- m_on_call_ip = m_core.PC * 2;
+ m_on_call_ip = m_core->PC * 2;
  Thread * old = m_threads[m_cur_thread];
  for(unsigned int i = 0; i < 32; i++) {
- old->registers[i] = (*m_core.R)[i];
+ old->registers[i] = (*(m_core->R))[i];
  }
 
  if(0xc9c <= m_on_call_ip && m_on_call_ip <= 0xca4)
@@ -286,7 +287,7 @@ void ThreadList::OnSPRead(int SP_value)
 
 void ThreadList::OnSPWrite( int new_SP )
 {
- if( ! m_core.Flash->LooksLikeContextSwitch(m_core.PC*2))
+ if( ! m_core->Flash->LooksLikeContextSwitch(m_core->PC*2))
  return;
  m_phase_of_switch = (m_phase_of_switch==eWritten) ? eWritten2 : eWritten;
  m_last_SP_writen = new_SP;
@@ -309,7 +310,7 @@ void ThreadList::OnPop()
  return;
  }
  m_phase_of_switch = eNormal;
- int addr = m_core.PC * 2;
+ int addr = m_core->PC * 2;
  assert(0 <= m_cur_thread && m_cur_thread < (int) m_threads.size());
  Thread * old = m_threads[m_cur_thread];
  assert(m_on_call_sp != 0x0000);
diff --git a/src/hwstack.h b/src/hwstack.h
index 5f748b3..6bae0f1 100644
--- a/src/hwstack.h
+++ b/src/hwstack.h
@@ -64,10 +64,10 @@ class ThreadList
     int m_on_call_ip;  ///< Address in a routine calling the context-switch
     /// Currently running thread. (Thread index used for querying by GDB is in GdbServer.)
     int m_cur_thread;
-    AvrDevice & m_core;
+    AvrDevice* m_core;
 public:
 
-    ThreadList(AvrDevice & core);
+    ThreadList(AvrDevice* core);
     ~ThreadList();
     void OnReset();
     void OnCall();

Marek Pietrzak <mpie>
Group Member
Mon 12 Sep 2011 07:47:16 PM UTC, comment #2: 

Hi Marek,  what about pasting a simple "git diff" with your fixes ?

Yann Dirson <ydirson>
Mon 12 Sep 2011 06:14:19 PM UTC, comment #1: 

I had the same problem with g++ 4.5.2 on Ubuntu. Apparently other versions are more liberal to non-static reference members. Anyway using references this way is sort of abuse in my opinion, so I changed it to pointer where necessary and it works fine. I planned to submit it as a bug along with a patch but till now there has been no strong motivation

Marek Pietrzak <mpie>
Group Member
Sun 11 Sep 2011 08:42:09 PM UTC, original submission:  

Tries with g++-4.3 through g++-4.6.  Had to make this change, but still not sufficient, the build subsequently fails at link time (on debian testing), with:

libtool: compile:  g++-4.3 -DHAVE_CONFIG_H -I. -I/usr/include/python2.6 -Ipython -I/usr/include -g -O2 -Icmd -Iui -Ihwtimer -MT lib_pysimulavr_la-pysimulavr_wrap.lo -MD -MP -MF .deps/lib_pysimulavr_la-pysimulavr_wrap.Tpo -c pysimulavr_wrap.cpp  -fPIC -DPIC -o .libs/lib_pysimulavr_la-pysimulavr_wrap.o
hwstack.h: In member function 'ThreadList& ThreadList::operator=(const ThreadList&)':
hwstack.h:57: error: non-static reference member 'AvrDevice& ThreadList::m_core', can't use default assignment operator
pysimulavr_wrap.cpp: In function 'PyObject* _wrap_HWStack_m_ThreadList_set(PyObject*, PyObject*)':
pysimulavr_wrap.cpp:32586: note: synthesized method 'ThreadList& ThreadList::operator=(const ThreadList&)' first required here
make[3]: * [lib_pysimulavr_la-pysimulavr_wrap.lo] Error 1
make[3]: Leaving directory `/work/yann/elec/avr/simulavr/src'
make[2]: * [all-recursive] Error 1


diff --git a/src/hwstack.cpp b/src/hwstack.cpp
index eddd686..975d01b 100644
--- a/src/hwstack.cpp
+++ b/src/hwstack.cpp
@@ -28,6 +28,7 @@
 #include "avrmalloc.h"
 #include "flash.h"
 #include <assert.h>
+#include <cstdio>
 
 using namespace std;
 

Yann Dirson <ydirson>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by zfrdh (Posted a comment)
  • -email is unavailable- added by petrh (Posted a comment)
  • -email is unavailable- added by mpie (Posted a comment)
  • -email is unavailable- added by ydirson (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-05-20 zfrdh StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code