RJLRef: $CASE/gen/ver_11/gen11bugs.2k0403 From lechner@saturn.cs.uml.edu Mon Apr 3 19:50:32 2000 From: Bob Lechner Subject: Re: HGcurr is NULL after pr_ADD("view", HG, (..)HG_elt); (ntansala) To: ntansala@cs.uml.edu (Naiyana Tansalarak), she@saturn.cs.uml.edu (Shuiqing He ) Date: Mon, 3 Apr 2000 19:50:24 -0400 (EDT) Cc: 2ks523 This email has evolved from a response to the NULL HGcurr problem into a log of my updates to my genv11 checkout in $CASE/gen/ver-11/chgen/src and my ~/bde2alpha_rv/bde/* builds as they progressed tonight. (Fixes are not all finished buit I'll report more progress later.) Fixes to be applied locally to genv11 and to ~/bde2alpha_rv/bde/ (but not yet checked-in to $GENROOT or $BDEROOT) are described below: (1) adding two lines to fix the HGcurr=NULL problem in pr_add(HG,. (2) re-emphasizing the need to #define pr_add(..) as gen_pr_add(..) (3) addition of -DDEFINE NEW_VERSION to bde/pr_util's Imakefile, (4) removal of ctwang's multiply-declared key_value and PARSE, (5) Include a pr_add call post-condition and update HGcurr: (6) At the end are some remarks about gencpp. =========================================================== (1) adding two lines to fix the HGcurr=NULL problem in pr_add(HG,.,,): Re: HGcurr is NULL after pr_ADD("view", HG, (..)HG_elt); (ntansala) This seemed surprising, because I thought pr_add sets HGcurr to HG_elt (it does not): > 912 pr_add("94sbdeview",HG, (hcg_ptr)HG_elt); So I checked pr_load,c (from genv11 in ~lechner/bde2alpha_rv/bde/pr_util): The call chain is: pr_add calls pr_link(tbl_encoding) which calls insert_element, link_parent and/or link_child. insert_element checks for empty HG_table ((HG*) HG==NULL) and adds HGelt there. The global HGelt is updated to the value of actual argument HG_elt. but unfortunately HGcurr is NOT assigned this value. To avoid messing up other users of global pointers HGelt and HGcurr, we ought to assign HGcurr = HG_elt (if desired) after return from pr_add, not inside it, if AND ONLY if we're sure that pr-add succeeded in linking HG_elt into the HG...HGend table list. That's not guaranteed, so we could instead assign HGcurr = HGelt, since HGelt is the global that pr_add updates. Before this, to ensure that HG_curr will not be updated to an older version of HGelt, we should assert pr_add's post-condition: assert(HGelt == HG_elt); /* pr_add post-condition */ HGcurr = HG_elt; /* it's safe to update HGcurr now */ In my opinion, this is a reasonable way to make sure that HGcurr != NULL OR that exit is called from assert(). -------------- /******************************************************************************/ /* This private macro will insert a row into a table. The pkey parameter is */ /* the name of the primary key field for the table in question. The row is */ /* linked into the tables row chain at the alphabetically correct location, */ /* sorted by primary key. */ /******************************************************************************/ #define insert_element(tbl,pkey) \ if (tbl == NULL) \ { \ tbl = tbl##elt; \ tbl##end = tbl##elt; \ } \ else \ if (key_compare(&tbl##end->pkey,&tbl##elt->pkey) < 0) \ ....other cases: insert into existing list between HG and HGend - RJL... -------------------------------------- ================================================= (2) re-emphasizing the need to #define pr_add(..) as gen_pr_add(..) However, it makes a diff whether pr_add macro calls gen_pr_add (vdhamoda, and genv11 TBupdated) or whether gen_pr_add is renamed pr-add (genv11, TBFixed). As I said in earlier messages, we NEED to retain or regain the redefinition (in schema.h) #define pr_add(a,b,c) gen_pr_add(a,#b, c) instead of renaming gen_pr_add as pr-add as in genv11, to avoid quoting arg2 in every pr_add call and messing up all other applications until they do the same repair. I have done this in ~lechner/bde2alpha_rv/bde/pr_util/pr_load.c at '/*RJL 2k0329:'. ===================================================== In his email below, ScottHe says: "in pr_load.c.apr : gen_pr_add() vdhamoda has commented the 2 line below ... and there are no[t] these 2 line in pr_log.c (pr_add) generated by chgenv11." I think he means that pr_util code from genv11 bypasses the view/version check. genv11 may not have considered the reason why this check existed because they threw out viewname as well as version number tests. Besides, another fix already was present in chgen: -DNEW_VERSION below. I'm sorry it took me this long to detect this old (1992!) work-around. Chgen version support is obsolete because it is not practical at the level of individual table rows. CVS is a better place for revision histories at the backup level, perhaps integrated with genlog's [distributed real-time?] transaction-level capabilities. Then byte two of pkey, which was version number, could expand the current 16-bit row number to 24 bits or 16M rows. Views as subschema remain valid for possible extensions of gen to subschemas which represent subsystem packages. THese can partition a large schema into separate namespaces (subschemas) so table_abbrevs (MSbyte of pkeys) can be reused in these separate namespace scopes. Example: bde, lcp, active and passive sub-application subschema as in the four quadrants of my Juice-Plant Data Model web-page slides. I inspected gen_pr_add in and found this comment re: NEW_VERSION that also bypasses the version test: ---------- gen_pr-add.c line 19: inserted NEWVERSION flag for 0 version problem -CEM 19/11/92 --------- gen_pr_add line 170: --------- #ifndef NEW_VERSION fprintf(prload_fp, "\ if (hcg_view_list.view_list[hcg_view_idx].version_list[hcg_tbl_idx] == ' \\0')\n\ return;\n\ "); #endif ------------ My conclusion is that (barring any other context that reverses the effect of NEW_VERSION definition) we only need to add -DNEW_VERSION to the chgen run command in pr_util/Imakefile to bypass this undesired test. (When genv11 runs, it also needs NEW_VERSION defined, as it is an application of pr_util from the metaschema. (The Makefile to build gen11 already does this - I modified it on 3/29: =================================================================== (4) removal of ctwang's multiply-declared key_value and PARSE, On making the NEW_VERSION changes, new errors came up: -------- [tern1](69)> pwd /nfs/jupiter/proj3/case/gen/ver_11/chgen/src [tern1](71)> make gcc -ansi -g -DDEBUG -DNEW_VERSION -c gen_defines.c ----------- The make run produced a new executable -rwxr-xr-x 1 lechner 98s523 1261568 Apr 3 17:57 chgen11 in /usr/proj3/case/gen/ver_11/chgen/bin/alpha. Before bde/src would compile I had to change line 787 of bde's schema.h from extern void pr_add(...) to extern void gen_pr_add(...) Now I get these errors when linking (via g++) every bde/src/*.cc: ----------- g++ -o bde -O2 -L/usr/lib bendpt.o ... collect2: ld returned 1 exit status /usr/ucb/ld: bendptops.o: key_value: multiply defined bendptops.o: PARSE: multiply defined bde.o: key_value: multiply defined ...... text.o: PARSE: multiply defined textops.o: key_value: multiply defined textops.o: PARSE: multiply defined Unresolved: logwait logstr ----------- I inspected schema.h and fond PARSE and key_value declared by ctwang but never used; therefore I'll delete it from #$CASE/gen/ver_11/genv11/src/gen_defines.c. logwait and logstr are both referenced in pr_{delete/free/load,log}.c; BOth are defined in pr_log: pr_log.c:344:void logwait() pr_log.c:324:void logstr(text) but pr_log is not in the src and obj file list of Imakefile. TBD: add them. (pr_log.c is only generated by genv11.) ------------------------------------------------------ PARSE and key_value only appear in a short range of bde/pr_util/*schema.h: line 291: /* BEGIN MODIFICATION, ctwang, 12/20/97 */ --------- jupiter.cs.uml.edu(135)> grep PARSE *.h *.c 94sbde_schema.h:294:int PARSE; 94sbde_schema.h:298: if((tbl)->RFLAG==1 && !PARSE && hcg_log ==1 )\ 94sbde_schema.h:308: if((tbl)->RFLAG==1 && !PARSE && hcg_log ==1 )\ 94sbde_schema.h:318: if((tbl)->RFLAG==1 && !PARSE && hcg_log ==1 )\ 94sbde_schema.h:332: if((tbl)->RFLAG==1 && !PARSE && hcg_log ==1 )\ jupiter.cs.uml.edu(136)> grep key_value *.h *.c 94sbde_schema.h:293:hcg_key key_value; /* for pr_set_key() */ 94sbde_schema.h:320: key_value = ( value ); \ ----------- line 340: /* END MODIFICATION, ctwang, 12/20/97 */ ================================================= (5) Include a pr_add call post-condition and update HGcurr: ScottHe and NTansalarak reported that bde crashed after File/New because HGcurr = NULL was passed to Caption_Create during GraphTitle entry. I will add the two statements assert(HGelt == HG_elt); HGcurr = HG_elt; after the pr-add(HG...) call in bde/src/?. This will ensure the post-condition that the newly inserted row at HG_elt is actually the pr_created struct at HGelt, and that HGcurr is set to that result. I intend to test these fixes in ~/bde2alpha_rv/bdei/pr_util, but my last 3/29 tests there found btidx bugs I believe. ---------------------- Bob Lechner ===================================================== HGcurr==NULL Bug report: (from ntansala) > Dear Prof. Lechner, > > I have found out something that If I use pr_*.c generated from chgen > 10.2, I will get the core dump as below : > > > gdb bde > GDB is free software and you are welcome to distribute copies of it > under certain conditions; type "show copying" to see the conditions. > There is absolutely no warranty for GDB; type "show warranty" for details. > GDB 4.15.1 (alpha-dec-osf1), Copyright 1995 Free Software Foundation, > Inc... > (gdb) break CreateNewGraph > Breakpoint 1 at 0x12001ecbc: file fileio.cc, line 874. > (gdb) r > Starting program: /nfs/jupiter/proj3/case/bdelog2ks/ntansala/bde/src/bde > Using Gen v10.2 ... > GD defaults file not specified. Using BDE defaults > State Machine is not initialized > Using default viewdefs: ../lib/bdetest.viewdefs > Using default bdenew.dat: ../lib/bdenew.dat > Warning: unknown table (/*) found in scanned datafile ../lib/fonts.dat, > ignored. > Warning: unknown table (/*) found in scanned datafile ../lib/fonts.dat, > ignored. > Warning: unknown table () found in scanned datafile ../lib/fonts.dat, > ignored. > Warning: unknown table () found in scanned datafile ../lib/fonts.dat, > ignored. > > Breakpoint 1, CreateNewGraph (widget=0x1400b7400, client_data=0x0, > cbs=0x11ffff4d8) at fileio.cc:874 > 874 clearObjects(); > (gdb) n > 875 ReDraw(); > (gdb) n > 878 XtSetSensitive( leftmenu, True ); > (gdb) n > 881 XmStringGetLtoR( cbs->value, XmSTRING_DEFAULT_CHARSET, &title ); > (gdb) n > 887 NULL ); > (gdb) n > 890 if ( strcmp( d_gcjustify, "center" ) == 0 ) > (gdb) n > 891 center_x = (float)canvas_width/2.0; > (gdb) n > 897 if ( strcmp( d_gcloc, "bottom" ) == 0 ) > (gdb) n > 900 center_y = d_gcheight/2; > (gdb) n > 902 HG_elt = pr_create(HG); > (gdb) n > 903 pr_set_str(HG_elt, FSid, "FS010000"); > (gdb) n > 904 pr_set_str(HG_elt, HNid, "HN010000"); > (gdb) n > 906 pr_set_str(HG_elt, HGauthor, GetUserName()); > (gdb) n > 907 pr_set_str(HG_elt, HGcreated, GetCurrentDateAndTime()); > (gdb) n > 908 pr_set_str(HG_elt, HGlastmod, GetCurrentDateAndTime()); > (gdb) n > 909 pr_set_str(HG_elt, HGtitle, title); > (gdb) n > 912 pr_add("94sbdeview",HG, (hcg_ptr)HG_elt); > (gdb) n > 914 CG_elt = pr_create(CG); // create a graph caption 94FBDE > Javier/Kathy/Lon > (gdb) n > 915 pr_set_int( CG_elt, HGid, HGcurr->HGid ) ; > (gdb) n > > Program received signal SIGSEGV, Segmentation fault. > 0x12001f304 in CreateNewGraph (widget=0x1400b7400, client_data=0x0, > cbs=0x11ffff4d8) at fileio.cc:915 > 915 pr_set_int( CG_elt, HGid, HGcurr->HGid ) ; > (gdb) > > > > HG_elt is not NULL but HGcurr is NULL. When I change HGcurr to HG_elt in line 915, > it will be ok. But HG_elt->HGid is still NULL. So, it is not right to fix > it. > > However, if use vdhamoda's pr_load.c.apr and prdump.c.apr, it won't effect > the line 915 because the system will set HGcurr to be HG_elt and > generate HGid via gen_pr_add(). > > in pr_load.c.apr : gen_pr_add() vdhamoda has commented the 2 line below : > > if (hcg_view_list.view_list[hcg_view_idx].version_list[hcg_tbl_idx] == '\0') > return; > > and there are no these 2 line in pr_log.c (pr_add) generated by chgenv11. > > > > Best regards, > Naiyana > ============================================ (6) A note for future gencpp implementers: - RJL 2k0403 There is no use changing all pr_add calls now because conversion to gencpp will radically alter the context: when HG is a class, its method HG_elt->HG::pr_add won't need any args, - because HG (scope) and HG_elt (HG-object pointer) appear before the method name, and arg1 = viewName will be a static variable (pre_set using something like HG::useView(char* viewName)) which calls pr_set_string. The risk of pre-setting a viewName (subschema) to be used is that multi-user re-entrant code requires either a separate viewname associated with each user process or thread: ([user]--->[user2viewRelation]<----[view]) or else all users sharing a [Distributed] BDE collaboration must agree to use the same view. (So far bde hasn't used Views anyway so it's a low priority for us.)