Make changes nescessary to build IL asm grammar with Bison. #89704

kant2002 · 2023-07-31T03:35:05Z

To generate with Bison

cd src/coreclr/ilasm
yacc asmparse.y --output=prebuilt/asmparse.cpp

If bison is unacceptable due to licensing

cd src/coreclr/ilasm
byacc asmparse.y --output=prebuilt/asmparse.cpp

Thanks @hez2010 for the hint.

Related to #4776

ghost · 2023-07-31T03:35:16Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #4776

Author:	kant2002
Assignees:	-
Labels:	`area-ILTools-coreclr`, `community-contribution`
Milestone:	-

kant2002 · 2023-07-31T03:37:47Z

Would be great if my changes was tested with internal yacc, so while build infra prepare nescessary images and stuff we can test this little experiment

RIP-webmaster · 2023-07-31T07:44:25Z

Unrelated to this change, but do someone know where "Lexical tokens" entries in asmparse.grammar come from? Looks like some of them are not accurate, for example: ID - C style alphaNumeric identifier (e.g. Hello_There2) - IlAsm identifiers can also contain character "`" (backtick), while C-style identifiers can only contain letters, digits and underscores. Where the exact meaning of tokens is specified?

hez2010 · 2023-07-31T08:25:17Z

src/coreclr/ilasm/asmparse.y

+%token <int32>   INT32_V        /* 3425 0x34FA  0352  */
+%token <int64>   INT64_V        /* 342534523534534      0x34FA434644554 */


I would instead recommend INT32_T and INT64_T for naming.

I assume _T is token here. I use _V for value. but whatether, let maintainers decide.

My 2 cents are: I personally like _V for value. If it were _T I would expect a token representing the type, not a value.

"_V" is fine.

hez2010 · 2023-07-31T08:25:45Z

src/coreclr/ilasm/asmparse.h

@@ -314,6 +314,7 @@ class AsmParse : public ErrorReporter
    friend char* nextBlank(_In_ __nullterminated char*);
    friend int ProcessEOF();
    friend unsigned __int8* skipType(unsigned __int8* ptr, BOOL fFixupType);
+    friend unsigned corCountArgs(BinStr* args);


This is unnecessary.
Instead, add a static unsigned corCountArgs(BinStr* args); declaration in grammar_before.cpp.

See 424e9a2

hez2010 · 2023-07-31T08:26:06Z

src/coreclr/ilasm/grammar_after.cpp

@@ -1557,7 +1557,7 @@ void FixupTyPars(BinStr* pbstype)
    FixupTyPars((PCOR_SIGNATURE)(pbstype->ptr()),(ULONG)(pbstype->length()));
 }
 /**************************************************************************/
-static unsigned corCountArgs(BinStr* args)
+unsigned corCountArgs(BinStr* args)


Unnecessary change.

kant2002 · 2023-07-31T09:49:08Z

@RIP-webmaster for example how numbers as lexems are parsed is defined here.

runtime/src/coreclr/ilasm/grammar_after.cpp

Lines 1184 to 1229 in 2422429

    
           else if (IsDigit(curSym) 
        
               || (curSym == '.' && IsDigit(Sym(nextchar(curPos)))) 
        
               || (curSym == '-' && IsDigit(Sym(nextchar(curPos))))) 
        
           { 
        
               const char* begNum = curPos; 
        
               unsigned radix = 10; 
        
               neg = (curSym == '-');    // always make it unsigned 
        
               if (neg) curPos = nextchar(curPos); 
        
               if (Sym(curPos) == '0' && Sym(nextchar(curPos)) != '.') 
        
               { 
        
                   curPos = nextchar(curPos); 
        
                   radix = 8; 
        
                   if (Sym(curPos) == 'x' || Sym(curPos) == 'X') 
        
                   { 
        
                       curPos = nextchar(curPos); 
        
                       radix = 16; 
        
                   } 
        
               } 
        
               begNum = curPos; 
        
               { 
        
                   unsigned __int64 i64 = str2uint64(begNum, const_cast<const char**>(&curPos), radix); 
        
                   unsigned __int64 mask64 = neg ? UI64(0xFFFFFFFF80000000) : UI64(0xFFFFFFFF00000000); 
        
                   unsigned __int64 largestNegVal32 = UI64(0x0000000080000000); 
        
                   if ((i64 & mask64) && (i64 != largestNegVal32)) 
        
                   { 
        
                       yylval.int64 = new __int64(i64); 
        
                       tok = INT64; 
        
                       if (neg) *yylval.int64 = -*yylval.int64; 
        
                   } 
        
                   else 
        
                   { 
        
                       yylval.int32 = (__int32)i64; 
        
                       tok = INT32; 
        
                       if(neg) yylval.int32 = -yylval.int32; 
        
                   } 
        
               } 
        
               if (radix == 10 && ((Sym(curPos) == '.' && Sym(nextchar(curPos)) != '.') || Sym(curPos) == 'E' || Sym(curPos) == 'e')) 
        
               { 
        
                   unsigned L = (unsigned)(PENV->endPos - begNum); 
        
                   curPos = (char*)begNum + GetDouble((char*)begNum,L,&yylval.float64); 
        
                   if (neg) *yylval.float64 = -*yylval.float64; 
        
                   tok = FLOAT64; 
        
               } 
        
           }

Identifier rules

runtime/src/coreclr/ilasm/grammar_after.cpp

Lines 548 to 558 in 2422429

    
               _ValidCS[(unsigned char)'_'] = TRUE; 
        
               _ValidCS[(unsigned char)'?'] = TRUE; 
        
               _ValidCS[(unsigned char)'$'] = TRUE; 
        
               _ValidCS[(unsigned char)'@'] = TRUE; 
        
               _ValidCS[(unsigned char)'`'] = TRUE; 
        
           } 
        
           BOOL IsAlpha(unsigned x) { return (x < 128)&&_Alpha[x]; } 
        
           BOOL IsDigit(unsigned x) { return (x < 128)&&_Digit[x]; } 
        
           BOOL IsAlNum(unsigned x) { return (x < 128)&&_AlNum[x]; } 
        
           BOOL IsValidStartingSymbol(unsigned x) { return (x < 128)&&_ValidSS[x]; } 
        
           BOOL IsValidContinuingSymbol(unsigned x) { return (x < 128)&&_ValidCS[x]; }

which is used here

runtime/src/coreclr/ilasm/grammar_after.cpp

Lines 903 to 918 in 2422429

    
               if (IsValidStartingSymbol(curSym)) 
        
               { // is it an ID 
        
           Its_An_Id: 
        
                   size_t offsetDot = (size_t)-1; // first appearance of '.' 
        
                           size_t offsetDotDigit = (size_t)-1; // first appearance of '.<digit>' (not DOTTEDNAME!) 
        
                   do 
        
                   { 
        
                       curPos = nextchar(curPos); 
        
                       if (Sym(curPos) == '.') 
        
                       { 
        
                           if (offsetDot == (size_t)-1) offsetDot = curPos - curTok; 
        
                           curPos = nextchar(curPos); 
        
                           if((offsetDotDigit==(size_t)-1)&&(Sym(curPos) >= '0')&&(Sym(curPos) <= '9')) 
        
                                   offsetDotDigit = curPos - curTok - 1; 
        
                       } 
        
                   } while(IsValidContinuingSymbol(Sym(curPos)));

all of that pretty standard lexer stuff encoded in the code directly.

hez2010 · 2023-07-31T14:56:57Z

src/coreclr/ilasm/grammar_before.cpp

@@ -51,6 +51,7 @@ static char* newString(_In_ __nullterminated const char* str1);
 static void corEmitInt(BinStr* buff, unsigned data);
 static void AppendStringWithLength(BinStr* pbs, _In_ __nullterminated char* sz);
 static void AppendFieldToCustomBlob(BinStr* pBlob, _In_ BinStr* pField);
+static unsigned corCountArgs(BinStr* args);


Also append below lines after this line:

extern void yyerror(_In_ __nullterminated const char*); extern Instr* SetupInstr(unsigned short); extern int yylex();

See 3bf3d70#diff-45997acd93aca6458c6a5b1ff5ebcc1eb2953ccff2062309d4c8592f366d0384

Can you clarify why extern needed?

It's optional, you don't have to add the extern.

hez2010

LGTM

hez2010 · 2023-08-03T16:07:11Z

src/coreclr/ilasm/asmparse.y

+%token <int32>   INT32_V        /* 3425 0x34FA  0352  */
+%token <int64>   INT64_V        /* 342534523534534      0x34FA434644554 */


We don't have to change it if we use byacc (https://invisible-island.net/byacc/byacc.html) instead of bison.
And worth to note by using bison there will be license conflict between GPL and MIT.
So I would recommend to revert this change (the parser generated by byacc doesn't have issue around here) and use byacc instead.

BISON Conditions

I'm not a lawyer. @richlander , what do you think? This only applies to the parser that BISON generates from the grammar.

Reverting this can help prevent developers from generating code using bison by accident (because it would just fail), which can make sure we won't have license issues.

I think it would be better to use byacc due to the licensing personally.

JulieLeeMSFT · 2023-09-21T20:45:13Z

@TIHan please review this community PR.

TIHan

Overall, this looks like a very simple change, though I do not know what the implications are with BISON being GPL and what the means for the parsers that are generated.

We need to get some insight with @richlander .

TIHan · 2023-10-13T20:25:21Z

src/coreclr/ilasm/asmparse.y

+%token <int32>   INT32_V        /* 3425 0x34FA  0352  */
+%token <int64>   INT64_V        /* 342534523534534      0x34FA434644554 */


"_V" is fine.

TIHan · 2023-10-13T20:31:17Z

src/coreclr/ilasm/asmparse.y

+%token <int32>   INT32_V        /* 3425 0x34FA  0352  */
+%token <int64>   INT64_V        /* 342534523534534      0x34FA434644554 */


BISON Conditions

I'm not a lawyer. @richlander , what do you think? This only applies to the parser that BISON generates from the grammar.

TIHan · 2023-10-13T20:43:34Z

@kant2002 , we are going to check to see if the generated parser from BISON will be license compatible. If it isn't, will byacc work?

ghost · 2023-10-28T09:01:11Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2023-11-11T12:01:18Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

TIHan · 2024-01-10T01:02:53Z

Re-opening, we can use this as long as we don't remove the special exception that BISON generates, very important.

TIHan · 2024-01-10T01:08:19Z

@kant2002 we are willing to accept this. Can you generate the new parser and make it part of this PR?

TIHan · 2024-01-10T17:35:05Z

I unlocked the thread, sorry about that. I didn't realize it was locked.

kant2002 · 2024-01-11T13:27:23Z

Let me check. Give me couple days to find time for this one

TIHan · 2024-01-11T17:12:16Z

No worries and no rush.

JulieLeeMSFT · 2024-03-25T15:47:45Z

@kant2002, this is a kindly reminder.

@kant2002 we are willing to accept this. Can you generate the new parser and make it part of this PR?

dotnet-policy-service · 2024-04-08T21:05:01Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

dotnet-policy-service · 2024-04-23T00:26:04Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

Make changes nescessary to build grammar with Bison.

fdc13ca

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 31, 2023

dotnet-issue-labeler bot added the area-ILTools-coreclr label Jul 31, 2023

Fix build

203e66d

kant2002 changed the title ~~Make changes nescessary to build grammar with Bison.~~ Make changes nescessary to build IL asm grammar with Bison. Jul 31, 2023

build-analysis bot mentioned this pull request Jul 31, 2023

Tracking issue for CI build timeouts #76454

Closed

hez2010 suggested changes Jul 31, 2023

View reviewed changes

Fix

93a6b2c

hez2010 reviewed Jul 31, 2023

View reviewed changes

Fix compilation on Unix

e18d4b1

hez2010 approved these changes Jul 31, 2023

View reviewed changes

hez2010 reviewed Aug 3, 2023

View reviewed changes

JulieLeeMSFT assigned kant2002 and TIHan Aug 28, 2023

JulieLeeMSFT added this to the 9.0.0 milestone Sep 5, 2023

BruceForstall requested a review from TIHan September 13, 2023 05:23

TIHan suggested changes Oct 13, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 13, 2023

ghost added the no-recent-activity label Oct 28, 2023

ghost closed this Nov 11, 2023

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

TIHan reopened this Jan 10, 2024

ghost removed the no-recent-activity label Jan 10, 2024

dotnet unlocked this conversation Jan 10, 2024

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 11, 2024

JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 25, 2024

dotnet-policy-service bot added the no-recent-activity label Apr 8, 2024

dotnet-policy-service bot removed this from the 9.0.0 milestone Apr 23, 2024

dotnet-policy-service bot closed this Apr 23, 2024

hez2010 mentioned this pull request Apr 23, 2024

Use bison for ilasm parser generator #101415

Merged

github-actions bot locked and limited conversation to collaborators May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes nescessary to build IL asm grammar with Bison. #89704

Make changes nescessary to build IL asm grammar with Bison. #89704

kant2002 commented Jul 31, 2023 •

edited

Loading

ghost commented Jul 31, 2023

kant2002 commented Jul 31, 2023

RIP-webmaster commented Jul 31, 2023

hez2010 Jul 31, 2023

kant2002 Jul 31, 2023

hamarb123 Jul 31, 2023

TIHan Oct 13, 2023

hez2010 Jul 31, 2023 •

edited

Loading

hez2010 Jul 31, 2023

kant2002 commented Jul 31, 2023

hez2010 Jul 31, 2023 •

edited

Loading

kant2002 Jul 31, 2023

hez2010 Jul 31, 2023

hez2010 left a comment

hez2010 Aug 3, 2023 •

edited

Loading

TIHan Oct 13, 2023 •

edited

Loading

hez2010 Oct 14, 2023

hamarb123 Feb 26, 2024

JulieLeeMSFT commented Sep 21, 2023

TIHan left a comment

TIHan Oct 13, 2023

TIHan Oct 13, 2023 •

edited

Loading

TIHan commented Oct 13, 2023

ghost commented Oct 28, 2023

ghost commented Nov 11, 2023

TIHan commented Jan 10, 2024

TIHan commented Jan 10, 2024

TIHan commented Jan 10, 2024

kant2002 commented Jan 11, 2024

TIHan commented Jan 11, 2024

JulieLeeMSFT commented Mar 25, 2024

dotnet-policy-service bot commented Apr 8, 2024

dotnet-policy-service bot commented Apr 23, 2024

		%token <int32> INT32_V /* 3425 0x34FA 0352 */
		%token <int64> INT64_V /* 342534523534534 0x34FA434644554 */

Make changes nescessary to build IL asm grammar with Bison. #89704

Make changes nescessary to build IL asm grammar with Bison. #89704

Conversation

kant2002 commented Jul 31, 2023 • edited Loading

ghost commented Jul 31, 2023

kant2002 commented Jul 31, 2023

RIP-webmaster commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kant2002 commented Jul 31, 2023

hez2010 Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 left a comment

Choose a reason for hiding this comment

hez2010 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

TIHan Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulieLeeMSFT commented Sep 21, 2023

TIHan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

TIHan commented Oct 13, 2023

ghost commented Oct 28, 2023

ghost commented Nov 11, 2023

TIHan commented Jan 10, 2024

TIHan commented Jan 10, 2024

TIHan commented Jan 10, 2024

kant2002 commented Jan 11, 2024

TIHan commented Jan 11, 2024

JulieLeeMSFT commented Mar 25, 2024

dotnet-policy-service bot commented Apr 8, 2024

dotnet-policy-service bot commented Apr 23, 2024

kant2002 commented Jul 31, 2023 •

edited

Loading

hez2010 Jul 31, 2023 •

edited

Loading

hez2010 Jul 31, 2023 •

edited

Loading

hez2010 Aug 3, 2023 •

edited

Loading

TIHan Oct 13, 2023 •

edited

Loading

TIHan Oct 13, 2023 •

edited

Loading