Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

include paths and include directive tidy up #102

Open
felix-salfelder opened this issue Jan 3, 2022 · 26 comments
Open

include paths and include directive tidy up #102

felix-salfelder opened this issue Jan 3, 2022 · 26 comments

Comments

@felix-salfelder
Copy link
Member

Currently admsXml adds '.' (current working directory) to the back of the include path list. This seems wrong and unusual. The C preprocessor has a more well behaved include directive, and does not do this.

NB: C has two different includes (quotes vs angle brackets) and the comparison might be inappropriate. It would be better to compare with some other verilog tool.

NB2: The reason the extra '.' has been added is probably related to the generation of header files (in '.'). This only caused problems (due to race conditions) and is no longer needed.

As a start, the implicit '.' should be removed, and instead specified by the user ('-I.') on demand. Then, less importantly, include directives should be resolved relative to the file they occur in, such that files in the same directory are found (unless found in a path specified by -I).
@felix-salfelder felix-salfelder mentioned this issue Jan 3, 2022
Closed
@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022

Hi Felix.

It is actually at the front still.

Current search order for devel as of 351f20e is

  1. current working directory;
  2. command line include directories;
  3. ADMS include directory.

This is what we want, I think.

@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022

Tested by putting BROKEN constants.vams file in current working directory.

bash> admsXml -x dummy.va 
[info...] -x: skipping any implicit xml scripts
[info...] admsXml-2.3.7 (unknown) Jan  2 2022 01:08:04
[fatal..] constants.vams: during lexical analysis syntax error at line 36 -- see 'THIS_IS_A_BAD_LINE'
bash> admsXml -x -I./include dummy.va 
[info...] -x: skipping any implicit xml scripts
[info...] admsXml-2.3.7 (unknown) Jan  2 2022 01:08:04
[fatal..] constants.vams: during lexical analysis syntax error at line 36 -- see 'THIS_IS_A_BAD_LINE'
bash> rm constants.vams 
bash> admsXml -x -I./include dummy.va 
[info...] -x: skipping any implicit xml scripts
[info...] admsXml-2.3.7 (unknown) Jan  2 2022 01:08:04
[info...] elapsed time: 0 (second)
[info...] admst iterations: 0 (0 freed)

@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022

I see no issue with removing the implicit -I. though, especially as this is not a standard thing to do and may catch people out! I also like the idea of adding in the implicit relative search path from the file containing the include.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 3, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022 •
edited
Loading

Removing it makes sense. I know that doing this alone will cause the Xyce buildxyceplugin script and all of their internal Makefiles to break for multi-file models though. Worst case, they can just add an explicit -I. in the files and still be usable with previous versions of ADMS. (This would also be a good reason for buildxyceplugin to add an interface to -I; it's inconvenient not having one!)

It would be good to also implicitly look relative to the file being processed though, as that is fairly standard practice; e.g.,

`include "relative/include/file.vams"

should look relative to the file being proceed first (instead of the CWD) and then relative to any path declared by by the user with an explicit -I. All that is required to achieve this is to find a way to add the directory of the file being processed to the includePath list, which seems not too tricky. This would also mean that the Xyce build tools would continue to work unchanged.

@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022

Your -I patch + this fix + constants.vams is a good incentive to make a release I reckon.

I would suggest we fix issue #39 too. The earliest version of disciplines.vams available from Accellera cannot be processed with ADMS, in part, because of this. I don't know enough about Bison to fix this myself (yet). There is also another syntax change that would need to be fixed; again, a Bison change.

@ngwood
Copy link
Contributor

ngwood commented Jan 3, 2022

I found a fix for issue #39 already. The last related thing I would suggest to get ADMS usable with the most recent Accellera standard library files is to get around their (presumably deliberate) use of an escaped identifier inside the disciplines.vams file. These are identifiers that are allowed to have extra special characters (not just $) in the name; namely any printable ASCII character, such as @, ", {, etc. This is a universal feature of the Verilog-AMS language, but no one actually uses this feature of the language as I don't think anyone supports it. Given that the offending identifier \logic (note the space at the end) is equivalent to the identifier logic (trailing space is irrelevant in free-form language) I suggest we do a nasty hack!

bash> git diff
diff --git a/admsXml/verilogaLex.l b/admsXml/verilogaLex.l
index ea1dfe4..4faabbb 100644
--- a/admsXml/verilogaLex.l
+++ b/admsXml/verilogaLex.l
@@ -274,6 +274,7 @@ INF {TKRETURN(yytext,yyleng); return tk_inf;}
 \|\| {TKRETURN(yytext,yyleng); return tk_or;}
 \^\~ {TKRETURN(yytext,yyleng); return tk_bitwise_equr;}
 
+\\{ident} {TKRETURN(yytext,yyleng); return tk_ident;}
 \${ident} {TKRETURN(yytext,yyleng); return tk_dollar_ident;}
 {char} {TKSTRIPPEDRETURN(yytext,yyleng); return tk_char;}
 {b8_int} {TKRETURN(yytext,yyleng); return tk_integer;}

It's not great but it's much better than not doing anything at all. We'd just need to document the heck out of this!

What do you think?

@tvrusso
Copy link

tvrusso commented Jan 3, 2022

Just weighing in here: I do not really like the suggestion of removing "search current directory" as a default behavior for ADMS, mostly because of the impact it will have on Xyce's use of ADMS.

Verilog-A models are often shipped as bundles of files that include each other in the same directory, and requiring an explicit "search this directory" command line option would be annoying given that it has always been the default behavior for as long as ADMS has existed.

Could we please just leave it as it is?

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 3, 2022 via email

@tvrusso
Copy link

tvrusso commented Jan 3, 2022

I will note that other Verilog-A compilers (Agilent's, for example) search for include files without a specified path by looking first in the same directory as the file being processed, then in any other search path given to the compiler.

If ADMS were changed to do that, I'd be behind it (though I'm not going to submit a PR to make it happen). But until that happens, please do not remove the default of looking in the current working directory first. Especially since this is long-standing behavior since the dawn of ADMS.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 3, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 17, 2022

This appears to be all that is needed for this.

diff --git a/admsXml/admsXml.c b/admsXml/admsXml.c
index 5d314b4..9dba440 100644
--- a/admsXml/admsXml.c
+++ b/admsXml/admsXml.c
@@ -615,7 +615,7 @@ 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_push(&pproot()->includePath,(p_adms)dirname(myverilogamsfile));
 #ifdef ADMS_INCLUDEDIR
     adms_slist_concat(&pproot()->includePath,adms_slist_new((p_adms)ADMS_INCLUDEDIR));
 #endif

This would make ADMS behave similarly to other Verilog-A compilers. I am fairly sure that this wouldn't interfere with how Xyce uses ADMS. The CWD could always be manually added with the -I. command line option.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 17, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 17, 2022

Ahh yes. This will not work with nested include files. It will only add the directory of the base Verilog-A file to the include path. I will give this some more thought.

@ngwood
Copy link
Contributor

ngwood commented Jan 17, 2022

The Verilog-AMS standard refers to the Verilog standard, which does not explicitly specify how relative paths should be evaluated.

From what I can see, any change would need to occur in the adms_preprocessor_lex_include_file function in preprocessorLex.l. It would be a lot of work to get this feature working though. It looks like it would be possible to go through each scanner of pproot()->Scanner and use the scanner->filename entries to construct temporary directory paths to push onto pproot()->includePath right before using adms_file_open_read_with_path that can be removed immediately after creating the file handle.

@ngwood
Copy link
Contributor

ngwood commented Jan 19, 2022 •
edited
Loading

I think this does what you're after.

diff --git a/admsXml/preprocessorLex.l b/admsXml/preprocessorLex.l
index 565ae9f..884bdbf 100644
--- a/admsXml/preprocessorLex.l
+++ b/admsXml/preprocessorLex.l
@@ -33,6 +33,8 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "admsPreprocessor.h"
 #include "preprocessorYacc.h"
 
+char* dirname(const char* myname);
+
 #ifndef INITIAL
 #  define INITIAL 0
 #endif
@@ -133,12 +135,20 @@ static void adms_preprocessor_lex_include_file (char* fileName)
   FILE*myfh;
   p_preprocessor scanner;
   char*message=NULL;
+  char*mydir=NULL;
+  p_slist l; for(l=pproot()->Scanner;l;l=l->next)
+  {
+    adms_k2strconcat(&mydir,dirname(((p_preprocessor)l->data)->filename));
+    adms_k2strconcat(&mydir,ADMS_PATH_SEPARATOR);
+  }
+  adms_k2strconcat(&mydir,dirname(pproot()->cr_scanner->filename));
   adms_k2strconcat(&message,pproot()->cr_scanner->filename);
   adms_k2strconcat(&message,":");
   adms_strconcat(&message,adms_integertostring(adms_preprocessor_get_line_position (pproot()->cr_scanner, 0)));
   if(pproot()->cr_filename)
     free(pproot()->cr_filename);
   pproot()->cr_filename=strdup(fileName);
+  adms_slist_push(&pproot()->includePath,(p_adms)mydir);
   if(!(myfh=adms_file_open_read_with_path(fileName,(p_slist)(pproot()->includePath))))
   {
     if(!strcmp(fileName,"discipline.h")||!strcmp(fileName,"disciplines.h")||!strcmp(fileName,"discipline.vams")||!strcmp(fileName,"disciplines.vams"))
@@ -161,6 +171,7 @@ static void adms_preprocessor_lex_include_file (char* fileName)
     else
       adms_message_fatal(("[%s]: failed to open file ... '%s'\n",message,fileName))
   }
+  adms_slist_pull(&pproot()->includePath);
   scanner=(p_preprocessor)malloc(sizeof(t_preprocessor));
   adms_message_verbose(("include file '%s'\n",fileName))
   scanner->buffer=NULL;
@@ -182,6 +193,7 @@ static void adms_preprocessor_lex_include_file (char* fileName)
   adms_k2strconcat(&preprocessorlval.mystr,"\"\n");
   BEGIN( INITIAL );
   free(message);
+  free(mydir);
 }
 
 static char *adms_preprocessor_lex_skipp_text ()

I think the implicit extern declaration for dirname in admsPreprocessor.h masks the explicit static keyword attached to the definition in admsXml.c.

Here is an example of it working.

$ export adms_verbose="yes"
$ tree dir1/
dir1/
├── dummy.va
└── include1
    ├── include1.va
    ├── include2
    │   └── include2.va
    └── include3
        └── include3.va

3 directories, 4 files
$ cat dir1/dummy.va 
// dummy.va

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

`include "include1/include1.va"
//`include "include3/include3.va"

module dummy(a, b);
    inout a, b;
    electrical a, b;
endmodule
$ cat dir1/include1/include1.va 
`define A 1.0
`include "include2/include2.va"
$ cat dir1/include1/include2/include2.va 
`define A 1.0
$ cat dir1/include1/include3/include3.va 
`define C 3.0
$ admsXml dir1/dummy.va 
[verbose] shift: -f dir1/dummy.va
[info...] admsXml-2.3.7 (unknown) Jan 19 2022 19:07:38
[verbose] define macro ... 'insideADMS'
[verbose] create temporary file .dummy.va.adms
[verbose] include file 'constants.vams'
[verbose] include file 'disciplines.vams'
[verbose] include file 'include1/include1.va'
[verbose] include file 'include2/include2.va'
[warning] pragma redefined ... 'A'
[verbose] No error found during parsing
[verbose] -e file: .adms.implicit.xml
[verbose] traverse: .adms.implicit.xml
[verbose] .interface.xml: file created (all -e files in one file)
[info...] elapsed time: 0 (second)
[info...] admst iterations: 2225 (364 freed)

If you uncomment the include for include3/include3.va in dir1/dummy.va then you will correctly get a fatal error because it is not supposed to look in dir1/include1.

[fatal..] [dir1/dummy.va:7]: failed to open file ... 'include3/include3.va'

@ngwood
Copy link
Contributor

ngwood commented Jan 23, 2022

I'm not 100% sure on whether this will work on Windows yet. Also, the dirname function had already been defined statically in admsXml.c meaning it's not defined in library admsElement like almost all other functions of this kind are. admsXml.c is used to create admsXml; but if you ever wanted to link against the admsPreprocessor library elsewhere you'd run into problems. Is it worth relocating the dirname and dependent functions? They could be made part of mkelements.pl. I'm thinking it would be safer and more consistent. Could we rename dirname to adms_dirname at the same time perhaps, as dirname is a standard library (string.h) function name?

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 24, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 24, 2022

I was mostly suggesting this for consistency. I did test that it works. I'm now thinking it's better if we don't, given what I know now.

On Windows, the win32_interface is a typedef of __declspec(dllexport) or __declspec(dllimport), depending on whether the function is being built into or accessed from a DLL, respectively; because the dirname function defined in admsXml.c never ends up in a DLL, there wouldn't be any issue for Windows users on this front. When I say I'm not 100% sure, it's because I don't have a Windows machine to test this with.

We only use libadmsPreprocessor for building admsXml (from admsXml.c), so we really don't need to make it available when building the other libraries, which is what putting it into libadmsElement would achieve. We could remove the static scope identifier from the dirname declaration in admsXml.c to make it obvious that this symbol is potentially used elsewhere, I guess. I suspect why I can get away with leaving it defined as static is because admsXml is built from a single file and libadmsPreprocessor is only ever used for building this one file.

@ngwood
Copy link
Contributor

ngwood commented Jan 24, 2022

I think #109 as it stands (https://github.com/Qucs/ADMS/pull/109/files) is the best solution.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 24, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 24, 2022 •
edited
Loading

Ahh! You are correct that libadmsPreprocessor, in my patch, it not using the dirname from admsXml.c at all; it must somehow be finding dirname(3). We could declare the ADMS dirname in a common location and rename it adms_dirname as I previously suggested; putting it in libadmsElement sounds sensible as it is already used when building both libadmsPreprocessor and admsXml. There are quite a few static function in admsXml.c. We would only really need to move those related to file path manipulation. I will update my pull request.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 24, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 24, 2022

I think the static functions in admsXml.c are there for no other reason than that is the only file they are used in. They were almost all ADMS-specific functions. The problem was I needed to use the ADMS dirname function (which we already know works well and across multiple platforms) but it was out of scope. All I've done is enlarge the scope. The ADMS version of dirname is doing a similar thing to the POSIX function, but it a very ADMS way; I'm not sure what the consequences are of replacing it are. I wouldn't want to make any more changes myself at this stage as I'm reasonably sure I haven't broken anything at this point.

One thing to bare in mind is that paths in Verilog-A source code will use the forward slash as the path separator. Cygwin/MinGW versions of libgen appear to simultaneously handle forward and backward slashes, but not as robustly as one might want them to. I have no idea how a native Windows build might behave or whether libgen is available for a given toolset. Selecting how and when to use adms_dirname also requires extra work.

There is one notable difference. The adms_dirname will always behave is the same way, which is ever so slightly different than the POSIX dirname function; i.e., it normalises the file path to only use ADMS_PATH_SEPARATOR.

@felix-salfelder
Copy link
Member Author

felix-salfelder commented Jan 25, 2022 via email

@ngwood
Copy link
Contributor

ngwood commented Jan 25, 2022

I see your point. I originally thought the interface change would be less bad than repeating code. If the intention is to refactor later on then I agree that it'd be better to add a static function that can be sorted out later; this way all of the changes needed to get this working are localised to just one file. I have updated my pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@felix-salfelder @tvrusso @ngwood