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

 
 

bug #34270: current master does no build

Submitted by:  Yann Dirson <ydirson>
Submitted on:  Sun 11 Sep 2011 08:42:09 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: ClosedComponent Version: simulavrxx

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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 File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 20 May 2013 02:57:57 PM UTCzfrdhStatusNone=>Fixed
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup