gnupic: Thread: Re: [gnupic] Re: gpasm `extern' vs `global' (new feature patch)

Craig Franklin writes ("[gnupic] Re: gpasm `extern' vs `global' (new feature patch)"):
> Ian Jackson wrote:
> >I'd be grateful if you'd let me know what you think of [my patch],
> >and I would be very happy if it were included in future gputils
> >versions.
> 
> It seems like a reasonable request.  Give me a few days to run some 
> tests and think about the consequences.

Thanks.

> >The patch was made against Debian's gputils 0.13.1-1.  This will be
> >very similar to upstream 0.13.1.  Note though that due to a bug in
> >0.13.1-1 I'm actually using 0.12.4-1 to which the patch also applies
> >cleanly.
> 
> Are you referring to this?
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=305547

Yes.  Sorry for not giving a reference earlier, but I hadn't had the
bug back from the Debian BTS.

> I don't generally review gputils bug reports on the debian tracker.  In 
> the future, it would be better if the bug was added to gputils 
> sourceforge or reported to me.

Certainly I wouldn't expect you to follow the Debian bugtracker.  The
Debian maintainer is supposed to pass on bug reports to the upstream
author/maintainer (when they're not specific to the Debian version).
Some upstreams have become rather upset by getting bugs from Debian
users about versions of the package that the upstream feels (often
rightly, IMO!) have been unhelpfully mangled.

But since you ask, I'll mail you directly next time.  I don't think
the bug in Debian #305547 is Debian-specific.

> I will try to take a look at the bug tomorrow.

Thanks.  Let me know if there's anything else I can do.

Ian.

In April, I wrote:
> I found it inconvenient that when defining a global symbol (one with
> `external linkage' in C parlance, although there is of course no C
> here) in gpasm I have to say `global', but when I'm referring to it I
> have to say `extern'.  That means I can't have one include file with a
> list of all of the global symbols.

And in October I wrote:
> So, after some head-scratching, I came up with an alternative - see
> below.  With this new patch you have to say `extern' _after_ the
> definition.  If you do, it treats the `extern' more or less as if it
> was `global'.
> 
> This is a less friendly version of the patch than the previous one,
> IMO, but the way the coff symbol number calculation is done makes it
> impractical do it it another way.

I've been using my patched 0.13.3-1 gputils for the past few months
and this has been working well.  I would like to suggest that my
patch, or something like it, be included in the next gputils release.

Thanks,
Ian.

diff -ru orig/gputils-0.13.3/debian/changelog gputils-0.13.3/debian/changelog
--- orig/gputils-0.13.3/debian/changelog	2005-10-28 00:02:21.754919510 +0100
+++ gputils-0.13.3/debian/changelog	2005-10-29 17:24:17.498756153 +0100
@@ -1,3 +1,13 @@
+gputils (0.13.3-1.0iwj2) unstable; urgency=low
+
+  * extern/global feature for labels:
+    If we encounter `extern' for a label which is already defined
+    as gvt_static or gvt_address, we change it to gvt_global.
+    This means that `extern' after a label definition is much like
+    `global'.
+
+ -- Ian Jackson ####@####.####  Sat, 29 Oct 2005 17:24:12 +0100
+
 gputils (0.13.3-1) unstable; urgency=low
 
   * New upstream version. 
Only in gputils-0.13.3/debian: changelog~
diff -ru orig/gputils-0.13.3/gpasm/coff.c gputils-0.13.3/gpasm/coff.c
--- orig/gputils-0.13.3/gpasm/coff.c	2005-01-03 22:21:04.000000000 +0000
+++ gputils-0.13.3/gpasm/coff.c	2005-10-29 15:59:18.913415094 +0100
@@ -447,7 +447,7 @@
     if ((new->type != class) || 
         (new->section_number != section_number)) {
       snprintf(message, sizeof(message),
-               "Duplicate label or redefining symbol that cannot be redefined. (%s)",
+               "Duplicate extern label or redefining symbol that cannot be redefined. (%s)",
                name);    
       gperror(GPE_UNKNOWN, message);
     }
@@ -455,7 +455,7 @@
 
   if ((new != NULL) && (type != gvt_extern) && (type != gvt_debug))  {
     snprintf(message, sizeof(message),
-             "Duplicate label or redefining symbol that cannot be redefined. (%s)",
+             "Duplicate non-extern label or redefining symbol that cannot be redefined. (%s)",
              name);    
     gperror(GPE_DUPLAB, message);
   } else {
diff -ru orig/gputils-0.13.3/gpasm/directive.c gputils-0.13.3/gpasm/directive.c
--- orig/gputils-0.13.3/gpasm/directive.c	2005-07-02 18:10:13.000000000 +0100
+++ gputils-0.13.3/gpasm/directive.c	2005-10-29 17:06:08.431816728 +0100
@@ -1342,6 +1342,8 @@
 		          struct pnode *parms)
 {
   char *p;
+  struct symbol *s;
+  struct variable *var;
   state.lst.line.linetype = dir;
   
   if (state.mode == absolute) {
@@ -1350,7 +1352,15 @@
     for (; parms; parms = TAIL(parms)) {
       p = maybe_evaluate_concat(HEAD(parms));
       if (p) {
-        set_global(p, 0, PERMANENT, gvt_extern);
+	s = get_symbol(state.stTop, p);
+	var = s ? get_symbol_annotation(s) : NULL;
+	if (var &&
+	    (var->previous_type == gvt_static || 
+	     var->previous_type == gvt_address)) {
+	  var->type = gvt_global;
+	} else {
+	  set_global(p, 0, PERMANENT, gvt_extern);
+	}
       }
     }
   }

On Fri, 2005-12-16 at 12:26 +0000, Ian Jackson wrote:


> I've been using my patched 0.13.3-1 gputils for the past few months
> and this has been working well.  I would like to suggest that my
> patch, or something like it, be included in the next gputils release.

Ian,

I like the idea of your patch. I too dislike having to doubly define
everything. The only concern I have with your patch is that it makes gpasm
incompatible with MPASM in some way. It doesn't appear that this is the
case, but do you know for sure? Just to be clear, your patch allows one to
write:

  EXTERN some_variable

This statement may reside in an include file which can be included by all
sources in the project; even the one that declares 'some_variable'.
Without your patch, gpasm will produce an error if the "EXTERN
some_variable" and "GLOBAL some_variable" statements are seen while
assembling a file.

I suppose if you write code for gpasm making use of this feature and then
attempt to assemble it with MPASM that it will fail? Is this true? Does it
matter?

Can someone with easy access to mpasm try assembling this:

;---- test file
  extern temp
  global temp

  end
;---- end test file

Scott

On Fri, 2005-12-16 at 12:26 +0000, Ian Jackson wrote:


> I've been using my patched 0.13.3-1 gputils for the past few months
> and this has been working well.  I would like to suggest that my
> patch, or something like it, be included in the next gputils release.

Ian,

I like the idea of your patch. I too dislike having to doubly define
everything. The only concern I have with your patch is that it makes gpasm
incompatible with MPASM in some way. It doesn't appear that this is the
case, but do you know for sure? Just to be clear, your patch allows one to
write:

  EXTERN some_variable

This statement may reside in an include file which can be included by all
sources in the project; even the one that declares 'some_variable'.
Without your patch, gpasm will produce an error if the "EXTERN
some_variable" and "GLOBAL some_variable" statements are seen while
assembling a file.

I suppose if you write code for gpasm making use of this feature and then
attempt to assemble it with MPASM that it will fail? Is this true? Does it
matter?

Can someone with easy access to mpasm try assembling this:

;---- test file
  extern temp
  global temp

  end
;---- end test file

Scott

Ian Jackson wrote:

>In April, I wrote:
>  
>
>>I found it inconvenient that when defining a global symbol (one with
>>`external linkage' in C parlance, although there is of course no C
>>here) in gpasm I have to say `global', but when I'm referring to it I
>>have to say `extern'.  That means I can't have one include file with a
>>list of all of the global symbols.
>>    
>>
>
>And in October I wrote:
>  
>
>>So, after some head-scratching, I came up with an alternative - see
>>below.  With this new patch you have to say `extern' _after_ the
>>definition.  If you do, it treats the `extern' more or less as if it
>>was `global'.
>>
>>This is a less friendly version of the patch than the previous one,
>>IMO, but the way the coff symbol number calculation is done makes it
>>impractical do it it another way.
>>    
>>
>
>I've been using my patched 0.13.3-1 gputils for the past few months
>and this has been working well.  I would like to suggest that my
>patch, or something like it, be included in the next gputils release.
>
>  
>

Please post a project that uses the new feature.  I want to make sure I 
understand how you are using it.

Microchip may have improved things in the last year, but IMHO they don't 
do enough to enforce symbol types and check for name space collisions.  
Back when I wrote gplink, I found out all sorts of neat things about 
mpasm.  They allowed the same symbol name to be local and static and 
also be extern (might have the details wrong, but you get the idea).  
Never figured out how they resolved which one would be used.  I sent 
several bug reports, but I don't think anything came of it.  Many hacks 
were added to gputils to work around the problems.

This history has me more than a little cautious about this change.  It 
looks innocent enough.  If this feature is added, there will have to be 
a command line option to enable it.  Another option is to create a new 
direction, unique to gpasm, to prevent confusion.  Regardless, I will 
wait to see your project before deciding on incorporating your patch.

I did add a feature to gpvo to help with this problem.  Don't remember 
if I told anyone.  There is a symbol export feature on gpvo.  It reads 
an object file and exports all the global symbols to a test file.  Of 
course you still have to include that file, but most of the hard work is 
done.  Should be simple enough to add to automate.  Circular references 
might be a problem.

>Thanks,
>Ian.
>
>diff -ru orig/gputils-0.13.3/debian/changelog gputils-0.13.3/debian/changelog
>--- orig/gputils-0.13.3/debian/changelog	2005-10-28 00:02:21.754919510 +0100
>+++ gputils-0.13.3/debian/changelog	2005-10-29 17:24:17.498756153 +0100
>@@ -1,3 +1,13 @@
>+gputils (0.13.3-1.0iwj2) unstable; urgency=low
>+
>+  * extern/global feature for labels:
>+    If we encounter `extern' for a label which is already defined
>+    as gvt_static or gvt_address, we change it to gvt_global.
>+    This means that `extern' after a label definition is much like
>+    `global'.
>+
>+ -- Ian Jackson ####@####.####  Sat, 29 Oct 2005 17:24:12 +0100
>+
> gputils (0.13.3-1) unstable; urgency=low
> 
>   * New upstream version. 
>Only in gputils-0.13.3/debian: changelog~
>diff -ru orig/gputils-0.13.3/gpasm/coff.c gputils-0.13.3/gpasm/coff.c
>--- orig/gputils-0.13.3/gpasm/coff.c	2005-01-03 22:21:04.000000000 +0000
>+++ gputils-0.13.3/gpasm/coff.c	2005-10-29 15:59:18.913415094 +0100
>@@ -447,7 +447,7 @@
>     if ((new->type != class) || 
>         (new->section_number != section_number)) {
>       snprintf(message, sizeof(message),
>-               "Duplicate label or redefining symbol that cannot be redefined. (%s)",
>+               "Duplicate extern label or redefining symbol that cannot be redefined. (%s)",
>                name);    
>       gperror(GPE_UNKNOWN, message);
>     }
>@@ -455,7 +455,7 @@
> 
>   if ((new != NULL) && (type != gvt_extern) && (type != gvt_debug))  {
>     snprintf(message, sizeof(message),
>-             "Duplicate label or redefining symbol that cannot be redefined. (%s)",
>+             "Duplicate non-extern label or redefining symbol that cannot be redefined. (%s)",
>              name);    
>     gperror(GPE_DUPLAB, message);
>   } else {
>diff -ru orig/gputils-0.13.3/gpasm/directive.c gputils-0.13.3/gpasm/directive.c
>--- orig/gputils-0.13.3/gpasm/directive.c	2005-07-02 18:10:13.000000000 +0100
>+++ gputils-0.13.3/gpasm/directive.c	2005-10-29 17:06:08.431816728 +0100
>@@ -1342,6 +1342,8 @@
> 		          struct pnode *parms)
> {
>   char *p;
>+  struct symbol *s;
>+  struct variable *var;
>   state.lst.line.linetype = dir;
>   
>   if (state.mode == absolute) {
>@@ -1350,7 +1352,15 @@
>     for (; parms; parms = TAIL(parms)) {
>       p = maybe_evaluate_concat(HEAD(parms));
>       if (p) {
>-        set_global(p, 0, PERMANENT, gvt_extern);
>+	s = get_symbol(state.stTop, p);
>+	var = s ? get_symbol_annotation(s) : NULL;
>+	if (var &&
>+	    (var->previous_type == gvt_static || 
>+	     var->previous_type == gvt_address)) {
>+	  var->type = gvt_global;
>+	} else {
>+	  set_global(p, 0, PERMANENT, gvt_extern);
>+	}
>       }
>     }
>   }
>
>  
>

> -----Original Message-----
> From: Scott Dattalo 
> Sent: Sunday, December 18, 2005 7:20 AM
> To: ####@####.####
> Subject: Re: [gnupic] Re: gpasm `extern' vs `global' (new 
> feature patch)
> 
> I suppose if you write code for gpasm making use of this 
> feature and then attempt to assemble it with MPASM that 
> it will fail? Is this  true? Does it matter?

I think it does matter. I would like to use the same code 
for MPASM/gpasm if possible.

> Can someone with easy access to mpasm try assembling this:
> 
> ;---- test file
>   extern temp
>   global temp
> 
>   end
> ;---- end test file
> 

It failed with the following error message (assemble only):

Executing: "C:\Program Files\Microchip\MPASM Suite\MPASMWIN.EXE" /q /p18F458
"test1.asm" /l"test1.lst" /e"test1.err" /o"test1.o"
Error[115]   C:\SOFTWARE\GPASM\TEST\TEST1.ASM 7 : 
Duplicate label ("temp" or redefining symbol that cannot be redefined)
BUILD FAILED: Wed Dec 21 09:59:00 2005

Regards.
Xiaofan

Scott Dattalo writes ("Re: [gnupic] Re: gpasm `extern' vs `global' (new feature patch)"):
> On Fri, 2005-12-16 at 12:26 +0000, Ian Jackson wrote:
> > I've been using my patched 0.13.3-1 gputils for the past few months
> > and this has been working well.  I would like to suggest that my
> > patch, or something like it, be included in the next gputils release.
> 
> I like the idea of your patch. I too dislike having to doubly define
> everything. The only concern I have with your patch is that it makes gpasm
> incompatible with MPASM in some way. It doesn't appear that this is the
> case, but do you know for sure? Just to be clear, your patch allows one to
> write:
> 
>   EXTERN some_variable
>
> This statement may reside in an include file which can be included by all
> sources in the project; even the one that declares 'some_variable'.

That's right.  The `extern' has to come after the definition,
unfortunately, but that's not _too_ inconvenient to manage usually -
just have a single file which you include everywhere at the end, with
all the `extern's.

> Without your patch, gpasm will produce an error if the "EXTERN
> some_variable" and "GLOBAL some_variable" statements are seen while
> assembling a file.

Exactly.

> I suppose if you write code for gpasm making use of this feature and then
> attempt to assemble it with MPASM that it will fail? Is this true? Does it
> matter?

Yes, it will fail (as has been reported by others).  That means that
code using this feature is not compatible with MPASM.  I don't see why
that should be an impediment to adding this feature ?  Many things
that make programming easier will not necessarily be supported by
MPASM.

Ian.

Craig Franklin writes ("Re: [gnupic] Re: gpasm `extern' vs `global' (new feature patch)"):
> Please post a project that uses the new feature.  I want to make sure I 
> understand how you are using it.

http://www.chiark.greenend.org.uk/ucgi/~ijackson/cvsweb/trains/
http://www.chiark.greenend.org.uk/~ian/d/trains-snapshot-2006-01-17.tar.gz

The nicest directory you want is probably the detpic one; the others
were tests and experiments.

Note that the files there don't have a uniform copyright status; for
ease of constructing my build system I've mixed in (eg) SPICE models
from semiconductor manufacturers and other things with restricted
licences.  So you should ask me before redistributing.

> Back when I wrote gplink, I found out all sorts of neat things about 
> mpasm.  They allowed the same symbol name to be local and static and 
> also be extern (might have the details wrong, but you get the idea).  

It's conventional for a toolchain to distinguish symbols local to each
file from global symbols with the same name; but often this is done by
having the local symbols not appear in the object file at all (since
they're not needed for linking).  I haven't looked at gputils's
arrangements for this in detail.

> This history has me more than a little cautious about this change.  It 
> looks innocent enough.  If this feature is added, there will have to be 
> a command line option to enable it.  Another option is to create a new 
> direction, unique to gpasm, to prevent confusion.  Regardless, I will 
> wait to see your project before deciding on incorporating your patch.

I have no objection to a new directive.  That would solve the problem
just as well.  But, note that this feature only applies new semantics
to what was previously an error so there is no need for an option to
turn it on (unless you think people will accidentally glue together
identically-named symbols in different files).

> I did add a feature to gpvo to help with this problem.  Don't remember 
> if I told anyone.  There is a symbol export feature on gpvo.  It reads 
> an object file and exports all the global symbols to a test file.  Of 
> course you still have to include that file, but most of the hard work is 
> done.  Should be simple enough to add to automate.  Circular references 
> might be a problem.

I have had some success with seddery of source files, but this (like
your suggestion) feels rather like a hack to someone who really
expects a toolchain to work the way the Unix C one does.

Thanks,
Ian.

In January 2006 I proposed a patch to gpasm to allow "extern" to work
like "global" for labels which have already been defined, rather than
giving an error.  This allows all the intentionall external symbols to
be collected together in one .inc file, as C programmers are used to.

The previous conversation ought to be archived here etc.:
  http://linuxhacker.org/cgi-bin/ezmlm-cgi?1:msp:4097:dlbeedhmomchhgdonnak
  http://linuxhacker.org/cgi-bin/ezmlm-cgi?1:ddn:4872:200512#b
see the thread
  gpasm `extern' vs `global' (new feature patch)
but currently that list archive seems partially broken.

> Craig Franklin wrote:
> > Please post a project that uses the new feature.  I want to make sure I 
> > understand how you are using it.
> 
> http://www.chiark.greenend.org.uk/ucgi/~ijackson/cvsweb/trains/
> http://www.chiark.greenend.org.uk/~ian/d/trains-snapshot-2006-01-17.tar.gz

The latter link is still valid although the project is no longer in
CVS.

> > [...]  Regardless, I will wait to see your project before deciding
> > on incorporating your patch.
> 
> I have no objection to a new directive.  That would solve the problem
> just as well.  But, note that this feature only applies new semantics
> to what was previously an error so there is no need for an option to
> turn it on (unless you think people will accidentally glue together
> identically-named symbols in different files).

I didn't hear any more about this.

I have just upgraded the host where I build my project to lenny and I
see that lenny's gpasm still does not support this feature.  I've once
again patched my copy.  Patch attached.

Aurelien, would you consider applying this change to squeeze's gpasm
in due course ?  If so I will file a wishlist bug.

Ian.


diff -ru orig/gputils-0.13.7/debian/changelog gputils-0.13.7/debian/changelog
--- orig/gputils-0.13.7/debian/changelog	2010-10-31 21:35:27.000000000 +0000
+++ gputils-0.13.7/debian/changelog	2010-10-31 21:36:39.000000000 +0000
@@ -1,3 +1,13 @@
+gputils (0.13.7-2.0iwj1) unstable; urgency=low
+
+  * extern/global feature for labels:
+    If we encounter `extern' for a label which is already defined
+    as gvt_static or gvt_address, we change it to gvt_global.
+    This means that `extern' after a label definition is much like
+    `global'.
+
+ -- Ian Jackson ####@####.####  Sun, 31 Oct 2010 21:36:39 +0000
+
 gputils (0.13.7-1) unstable; urgency=low
 
   * New upstream release.
Only in gputils-0.13.7/debian: changelog.orig
Only in gputils-0.13.7/debian: changelog~
diff -ru orig/gputils-0.13.7/gpasm/coff.c gputils-0.13.7/gpasm/coff.c
--- orig/gputils-0.13.7/gpasm/coff.c	2009-03-13 18:15:30.000000000 +0000
+++ gputils-0.13.7/gpasm/coff.c	2010-10-31 21:35:53.000000000 +0000
@@ -458,7 +458,7 @@
     if ((new->type != class) || 
         (new->section_number != section_number)) {
       snprintf(message, sizeof(message),
-               "Duplicate label or redefining symbol that cannot be redefined. (%s)",
+               "Duplicate extern label or redefining symbol that cannot be redefined. (%s)",
                name);    
       gperror(GPE_UNKNOWN, message);
     }
@@ -466,7 +466,7 @@
 
   if ((new != NULL) && (type != gvt_extern) && (type != gvt_debug))  {
     snprintf(message, sizeof(message),
-             "Duplicate label or redefining symbol that cannot be redefined. (%s)",
+             "Duplicate non-extern label or redefining symbol that cannot be redefined. (%s)",
              name);    
     gperror(GPE_DUPLAB, message);
   } else {
Only in gputils-0.13.7/gpasm: coff.c.orig
diff -ru orig/gputils-0.13.7/gpasm/directive.c gputils-0.13.7/gpasm/directive.c
--- orig/gputils-0.13.7/gpasm/directive.c	2009-03-13 18:15:30.000000000 +0000
+++ gputils-0.13.7/gpasm/directive.c	2010-10-31 21:35:53.000000000 +0000
@@ -1660,6 +1660,8 @@
 		          struct pnode *parms)
 {
   char *p;
+  struct symbol *s;
+  struct variable *var;
   state.lst.line.linetype = dir;
   
   if (state.mode == absolute) {
@@ -1668,7 +1670,15 @@
     for (; parms; parms = TAIL(parms)) {
       p = maybe_evaluate_concat(HEAD(parms));
       if (p) {
-        set_global(p, 0, PERMANENT, gvt_extern);
+	s = get_symbol(state.stTop, p);
+	var = s ? get_symbol_annotation(s) : NULL;
+	if (var &&
+	    (var->previous_type == gvt_static || 
+	     var->previous_type == gvt_address)) {
+	  var->type = gvt_global;
+	} else {
+	  set_global(p, 0, PERMANENT, gvt_extern);
+	}
       }
     }
   }
Only in gputils-0.13.7/gpasm: directive.c.orig

Aurelien Jarno writes ("Re: [gnupic] Re: gpasm `extern' vs `global' (new feature patch)"):
> I am not the maintainer anymore for almost two years, you should contact
> the new maintainer Taisuke Yamada ####@####.####

Ah, OK.  (I looked at the Maintainer field in lenny, sorry...)

Thanks,
Ian.