constants.vams and disciplines.vams

[ngwood]

The constants.vams and disciplines.vams files that are automatically created and placed into ${prefix}/include/adms during installation take precedence over files of the same name placed elsewhere in the search path of admsXml. This means that the only way to not use the broken versions of these files is to explicitly replace the ones in ${prefix}/include/adms. This wasn't at all obvious to me and it took me a while to work out why my local constants.vams file was not actually being read. Had there been a warning in the log file, similar to the one when admsXml is force to generate a local copy of these header files, it would have been obvious though. Better yet, could we not add and environment variable that lets the user set the location of their versions of constants.vams and disciplines.vams?

[ngwood]

I should also point out that the disciplines.vams ADMS suggests you use cannot be parsed by ADMS.

[felix-salfelder]

On Thu, Dec 30, 2021 at 12:55:56PM -0800, Neal wrote:

The constants.vams and disciplines.vams files that are automatically created and placed into ${prefix}/include/adms during installation take precedence over files of the same name placed elsewhere in the search path of admsXml.

Hi Neal.

Are you saying that despite passing -I/some/other/path to admsXml, a statement such as `include "disciplines.vams" picks up the file from ${prefix}/include/adms? I tend to agree that this would be bad behaviour.

Please give more details and describe some way to check, perhaps with a minimal example.

obvious though. Better yet, could we not add and environment variable

I suggest to find if anything about include directives and search paths is specified in the standard, rather than inventing environment variables. Imo environment variables are not a good approach to this. I guess modifying the default behaviour might be an option.

FWIW: Towards a sensible solution, we should look at what others do. C preprocessors do both includes with double quotes and includes with angle brackets -- to disambiguate in situations such as yours.

On Thu, Dec 30, 2021 at 01:28:13PM -0800, Neal wrote:

I should also point out that the disciplines.vams ADMS suggests you use cannot be parsed by ADMS.

Thanks for letting us know. Please give more details (open another ticket) or send a patch (e.g. merge request).

[ngwood]

Hi Felix.

I tried both placing a local copy of constants.vams and disciplines.vams into the same directory as the module file and using -I/some/other/path with admsXml; in both cases the ${prefix}/include/adms files were picked up by ADMS (v2.3.7). The reason I noticed this is because I was attempting to use a constant (M_TWO_PI) that is absent from the ADMS version of constants.vams.

This is a minimal working example.

// dummy.va

`include "constants.vams"
`include "disciplines.vams"

`define A `M_PI
`define B `M_TWO_PI

module dummy(a, b);
    inout a, b;
    electrical a, b;
    analog I(a, b) <+ V(a, b);
endmodule

bash> admsXml -x -I"${HOME}/Documents/accellera/include" dummy.va 
[info...] -x: skipping any implicit xml scripts
[info...] admsXml-2.3.7 (unknown) May  9 2021 20:28:22
[info...] [./dummy.va:7]: macro `M_TWO_PI is undefined
[info...] elapsed time: 0 (second)
[info...] admst iterations: 0 (0 freed)

Notice that M_TWO_PI is not found, but M_PI is, despite ${HOME}/Documents/accellera/include containing both.

I haven't looked into why ADMS is misbehaving yet as I have been a little too busy recently. I just don't want anyone else to get caught out by this.

[felix-salfelder]

On Fri, Dec 31, 2021 at 07:18:28AM -0800, Neal wrote:

[..] bash> admsXml -x -I${HOME}/Documents/accellera/include dummy.va [info...] -x: skipping any implicit xml scripts [info...] admsXml-2.3.7 (unknown) May 9 2021 20:28:22 [info...] [./dummy.va:5]: macro `M_TWO_PI is undefined [info...] elapsed time: 0 (second) [info...] admst iterations: 0 (0 freed)

Thanks, I can reproduce this.

It looks like ${prefix}/include/adms is added to the include paths before adding the -I values. Perhaps changing the order will improve the situation.

Send a patch if you like. Lets see if there is any protest (unlikely). Please add a test if you can.

[ngwood]

These are the offending lines of code from admsXml/admsXml.c.

     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
     adms_slist_push(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
     adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
 #endif

From what I can see, adms_slist_push adds to the front of the list.

void adms_slist_push (p_slist* l,p_adms data)
{
  p_slist n=malloc(sizeof(t_slist));
  n->next=*l;
  n->data=data;
  *l=n;
}

Thus it seems as though the list ends up as

1. ${prefix}/include/adms
2. .
3. -I/path/to/dir1
4. -I/path/to/dir2 etc.

NB: User include directory dir1 is before dir2 because dir1 is added first, then dir2 is added in front of dir1, then the list is reversed.

I would suggest creating an adms_slist_unshift function. I would have thought something like

void adms_slist_unshift (p_slist* l,p_adms data)
{
  p_slist n=malloc(sizeof(t_slist));
  n->next=NULL;
  n->data=data;
  adms_slist_last(*l)->next=n
}

along with

    pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
    adms_slist_unshift(&pproot()->includePath,(p_adms)".");
#ifdef ADMS_INCLUDEDIR
    adms_slist_unshift(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
#endif

would work to to give us

1. -I/path/to/dir1
2. -I/path/to/dir2 etc.
3. .
4. ${prefix}/include/adms

but it doesn't. Maybe someone with more insight into the ADMS build system could suggest where I am going wrong?

bash> admsXml -x dummy.va 
[info...] -x: skipping any implicit xml scripts
[info...] admsXml-2.3.7 (unknown) Dec 31 2021 18:47:55
Segmentation fault: 11

I was able to traverse the list fine, but when I don't use -I on the command line I get a segmentation fault.

[ngwood]

I can at least say that swapping the order resolves the issue.

diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index fa15213..21cda01 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -615,10 +615,12 @@ static void parseva (const int argc,const char** argv,char* myverilogamsfile)
     pproot()->error=0;
     adms_slist_push(&pproot()->skipp_text,(p_adms)(long)(0));
     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
+    adms_slist_inreverse(&pproot()->includePath);
     adms_slist_push(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
     adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
 #endif
+    adms_slist_inreverse(&pproot()->includePath);
     adms_preprocessor_get_define_from_argv(argc,argv);
     adms_preprocessor_define_add_default("insideADMS");
     adms_message_verbose(("create temporary file %s\n",mytmpverilogamsfile))

This is messy though.

[ngwood]

I now realise where I went wrong. This is what it should've been.

void adms_slist_unshift (p_slist* l,p_adms data)
{
  p_slist n=malloc(sizeof(t_slist));
  n->next=NULL;
  n->data=data;
  if (*l)
    adms_slist_last(*l)->next=n;
  else
    *l=n;
}

That is, if there is at least one path already in the list, then do as before; but if the list is empty, as it is when I don't use any -I on the command line, then make this new element the list.

bash> git diff
diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index fa15213..8d1d9aa 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -615,9 +615,9 @@ static void parseva (const int argc,const char** argv,char* myverilogamsfile)
     pproot()->error=0;
     adms_slist_push(&pproot()->skipp_text,(p_adms)(long)(0));
     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
-    adms_slist_push(&pproot()->includePath,(p_adms)".");
+    adms_slist_unshift(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
-    adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
+    adms_slist_unshift(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
 #endif
     adms_preprocessor_get_define_from_argv(argc,argv);
     adms_preprocessor_define_add_default("insideADMS");
diff --git a/admsXml/mkelements.pl b/admsXml/mkelements.pl
index b3ae052..25d98c8 100644
--- a/admsXml/mkelements.pl
+++ b/admsXml/mkelements.pl
@@ -418,6 +418,7 @@ win32_interface p_adms adms_slist_pull(p_slist* l);
 win32_interface void adms_slist_push(p_slist* l,p_adms data);
 win32_interface p_slist adms_slist_reverse (p_slist l);
 win32_interface void adms_slist_inreverse (p_slist* l);
+win32_interface void adms_slist_unshift(p_slist* l,p_adms data);
 struct s_slist
 {
   p_adms data;
@@ -1637,6 +1638,16 @@ void adms_slist_free (p_slist l)
     free(freed);
   }
 }
+void adms_slist_unshift (p_slist* l,p_adms data)
+{
+  p_slist n=malloc(sizeof(t_slist));
+  n->next=NULL;
+  n->data=data;
+  if (*l)
+    adms_slist_last(*l)->next=n;
+  else
+    *l=n;
+}
 int globalnbadmstnew=0, globalnbadmstdestroy=0;
 int adms_global_nbadmstnew (void) {return globalnbadmstnew;}
 int adms_global_nbadmstdestroy (void) {return globalnbadmstdestroy;}

[ngwood]

It would actually make more sense to search the current working directory first, then the command line paths, then finally the ADMS include directory.

1. .
2. -I/path/to/dir1
3. -I/path/to/dir2 etc.
4. ${prefix}/include/adms

diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index fa15213..2366aea 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -617,7 +617,7 @@ static void parseva (const int argc,const char** argv,char* myverilogamsfile)
     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
     adms_slist_push(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
-    adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
+    adms_slist_unshift(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
 #endif
     adms_preprocessor_get_define_from_argv(argc,argv);
     adms_preprocessor_define_add_default("insideADMS");
diff --git a/admsXml/mkelements.pl b/admsXml/mkelements.pl
index b3ae052..25d98c8 100644
--- a/admsXml/mkelements.pl
+++ b/admsXml/mkelements.pl
@@ -418,6 +418,7 @@ win32_interface p_adms adms_slist_pull(p_slist* l);
 win32_interface void adms_slist_push(p_slist* l,p_adms data);
 win32_interface p_slist adms_slist_reverse (p_slist l);
 win32_interface void adms_slist_inreverse (p_slist* l);
+win32_interface void adms_slist_unshift(p_slist* l,p_adms data);
 struct s_slist
 {
   p_adms data;
@@ -1637,6 +1638,16 @@ void adms_slist_free (p_slist l)
     free(freed);
   }
 }
+void adms_slist_unshift (p_slist* l,p_adms data)
+{
+  p_slist n=malloc(sizeof(t_slist));
+  n->next=NULL;
+  n->data=data;
+  if (*l)
+    adms_slist_last(*l)->next=n;
+  else
+    *l=n;
+}
 int globalnbadmstnew=0, globalnbadmstdestroy=0;
 int adms_global_nbadmstnew (void) {return globalnbadmstnew;}
 int adms_global_nbadmstdestroy (void) {return globalnbadmstdestroy;}

The issue was introduced by commit 2a76e2a on include branch.

[felix-salfelder]

On Fri, Dec 31, 2021 at 03:22:52PM -0800, Neal wrote:

It would actually make more sense to search the current working directory first, then the command line paths, then finally the ADMS include directory.

1. .
2. -I/path/to/dir1
3. -I/path/to/dir2 etc.
4. ${prefix}/include/adms

Reading the current code, one might think it is

1. -I/path/to/dir1
2. -I/path/to/dir2
3. .
4. ${prefix}/include/adms.

Perhaps "push" sounds more like "push_back", but actually implements "push_front"...?

[..] +win32_interface void adms_slist_unshift(p_slist* l,p_adms data);

If this is meant to push back, we better call it adms_slist_push_back. Otherwise I agree with the change, i.e.

ifdef ADMS_INCLUDEDIR

• adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
• adms_slist_push_back(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);

  endif

Thanks for figuring this out.

[ngwood]

I agree that the ADMS implementation of adms_slist_push is like a traditional unshift operation. The function I suggested is more like a traditional push operation. Therein lies the dilemma!

I think calling the alternative function adms_slist_push_back would be semantically meaningful. Perhaps adms_slist_append would be another good choice, but looses the fact that it no longer has push in the name.

That said, ADMS code currently seems to wilfully only use adms_slist_push and then just reverse the list afterwards when needed; this works but for long lists is inefficient. It may be a good idea, to remain consistent and to use the same approach here. It's unlikely that the includePath list will be very long, so, if you wanted, the least intrusive fix you can do, in good conscience, is to just add the following two lines to admsXml.c.

 #ifdef ADMS_INCLUDEDIR
+    adms_slist_inreverse(&pproot()->includePath);
     adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
+    adms_slist_inreverse(&pproot()->includePath);
 #endif

The first makes the end of the list the beginning of the list, after you have added ADMS_INCLUDEDIR to the front of the list you reverse the list again. Now the original front is the front again and ADMS_INCLUDEDIR is the last element, as intended. This is equivalent to using adms_slist_push_back but avoids the need for a new function to be defined, which likely will never get used for anything else.

[ngwood]

The alternative is to "push" ADMS_INCLUDEDIR into something other than pproot()->includePath, then join the temporary single element list to pproot()->includePath.

[ngwood]

This is actually the most elegant solution, requiring just one line of code to change.

diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index fa15213..5d314b4 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -617,7 +617,7 @@ static void parseva (const int argc,const char** argv,char* myverilogamsfile)
     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
     adms_slist_push(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
-    adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
+    adms_slist_concat(&pproot()->includePath,adms_slist_new((p_adms)ADMS_INCLUDEDIR));
 #endif
     adms_preprocessor_get_define_from_argv(argc,argv);
     adms_preprocessor_define_add_default("insideADMS");

[felix-salfelder]

On Sat, Jan 01, 2022 at 04:12:29AM -0800, Neal wrote:

This is actually the most elegant solution, requiring just one line of code to change.

diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index fa15213..5d314b4 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -617,7 +617,7 @@ static void parseva (const int argc,const char** argv,char* myverilogamsfile)
     pproot()->includePath=getlist_from_argv(argc,argv,"-I","directory");
     adms_slist_push(&pproot()->includePath,(p_adms)".");
 #ifdef ADMS_INCLUDEDIR
-    adms_slist_push(&pproot()->includePath,(p_adms)ADMS_INCLUDEDIR);
+    adms_slist_concat(&pproot()->includePath,adms_slist_new((p_adms)ADMS_INCLUDEDIR));
 #endif
     adms_preprocessor_get_define_from_argv(argc,argv);
     adms_preprocessor_define_add_default("insideADMS");

I agree. Please commit this :)