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.

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

Fix places where struct attributes should be permitted #193

Merged
merged 18 commits into from
Oct 21, 2022

Conversation

krame505
Copy link
Member

@krame505 krame505 commented Apr 7, 2022

It seems the best way of resolving the ambiguity is to use precedence - we want to prefer the shift (attaching attributes to the struct declaration specifier) over the reduce (putting attributes following a struct declaration specifier as their own declaration specifier.)

remexre and others added 3 commits April 4, 2022 10:49
…he C11 standard.

This hasn't been a problem historically, because if we have code like:

    if(foo) {
        do { ... } while(0);
    }

Then it gets parsed as (not the real AST):

    if($Expr{ foo }, seqStmt(
        doWhile(...), -- do { ... } while(0)
	emptyStmt(), -- ;
    ))

However, this caused a parse failure when parsing the DPDK header, with code like:

    if(foo)
        do { ... } while(0);
    else
        f();

In this code, we have two statements after the if, so the else is
"floating."
This is implemented synonym for C11 _Thread_local.
https://stackoverflow.com/a/40635676 suggests that they're the same?
This probably ought to be verified before merging this, but this is a
hotfix.
@krame505 krame505 requested a review from remexre April 7, 2022 20:22
@remexre
Copy link
Member

remexre commented Apr 7, 2022

@RandomActsOfGrammar can you check if this + the changes on the dpdk-bugs branch are enough to build your thing?

@krame505
Copy link
Member Author

krame505 commented Apr 7, 2022

This parses the minimal example you sent me, at least. Actually we should probably add that to the ableC test cases too.

@RandomActsOfGrammar
Copy link

It appears not, but it is also possible I didn't figure out all the build steps I needed to do.

I built AbleC with the changes, then I built Silver-AbleC which my extension uses (not sure if this is necessary), then I built my extension. I then tried to build my example, and I got an error from the DPDK code.

The error:

Error at line 55, column 8 in file /usr/lib/gcc/x86_64-pc-linux-gnu/11.2.0/include/ia32intrin.h
         (parser state: 102; real character index: 345542):
  Expected a token of one of the following types:
   [edu:umn:cs:melt:ableC:concretesyntax:Spaces_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:Pack_t,
    'redefine_extname']
   Input currently matches:
   [edu:umn:cs:melt:ableC:concretesyntax:Identifier_t,
    edu:umn:cs:melt:ableC:concretesyntax:TypeName_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:AttributeNameUnfetterdByKeywords_t]

The immediate context in that file:

#ifndef __SSE4_2__
#pragma GCC push_options
#pragma GCC target("sse4.2")
#define __DISABLE_SSE4_2__
#endif /* __SSE4_2__ */

Line 55 is the first #pragma there. Column 8 appear to be GCC.

@remexre
Copy link
Member

remexre commented Apr 7, 2022

can you do the gcc -E thing again and see where it appears there?

@RandomActsOfGrammar
Copy link

I ran gcc -E -x c generic_decomposed_nf.xc > out, then ran my composed AbleC on that. I still had the same error in the same included file, which suggests I'm not doing some step in the "gcc-E thing".

@krame505
Copy link
Member Author

krame505 commented Apr 7, 2022

Try deleting the CPP location tags? Otherwise it's still using the locations from the original file.

@remexre
Copy link
Member

remexre commented Apr 7, 2022

yeah that was the grep -v cmd from monday(?)

@krame505
Copy link
Member Author

krame505 commented Apr 7, 2022

Or sed -i'' '/# .*/d' out

@RandomActsOfGrammar
Copy link

After running Lucas's command, I'm getting the same error on the same line, but now with a location in the out file.

@remexre
Copy link
Member

remexre commented Apr 7, 2022

what's at that location now

@RandomActsOfGrammar
Copy link

#pragma GCC push_options

@RandomActsOfGrammar
Copy link

I don't remember this line from Monday.

@krame505
Copy link
Member Author

krame505 commented Apr 7, 2022

Huh, so it wasn't a weird location issue. We should be ignoring all unrecognized global pragmas, though. Or is this inside of a function? Could you send the exact parse error?

@RandomActsOfGrammar
Copy link

Parse error:

Error at line 12966, column 8 in file out
         (parser state: 102; real character index: 312605):
  Expected a token of one of the following types:
   [edu:umn:cs:melt:ableC:concretesyntax:Spaces_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:Pack_t,
    'redefine_extname']
   Input currently matches:
   [edu:umn:cs:melt:ableC:concretesyntax:Identifier_t,
    edu:umn:cs:melt:ableC:concretesyntax:TypeName_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:AttributeNameUnfetterdByKeywords_t]

It is not inside a function. I don't think it is inside anything. I don't know how to attach the whole file here.

@krame505
Copy link
Member Author

krame505 commented Apr 7, 2022

Okay, I guess we weren't handling all pragmas by default - try it again with my fix/struct-attributes branch

@RandomActsOfGrammar
Copy link

I'm still getting an error in the same place:

Error at line 55, column 8 in file /usr/lib/gcc/x86_64-pc-linux-gnu/11.2.0/include/ia32intrin.h
         (parser state: 102; real character index: 345542):
  Expected a token of one of the following types:
   [edu:umn:cs:melt:ableC:concretesyntax:Spaces_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:GCC_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:Pack_t,
    'redefine_extname']
   Input currently matches:
   [edu:umn:cs:melt:ableC:concretesyntax:Identifier_t,
    edu:umn:cs:melt:ableC:concretesyntax:TypeName_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:AttributeNameUnfetterdByKeywords_t]

@krame505
Copy link
Member Author

krame505 commented Apr 8, 2022

Oops, copy-pasted a regex without looking too closely - try it again.

@RandomActsOfGrammar
Copy link

The good news is you fixed that error. The bad news is there is another one:

Error at line 43, column 23 in file /usr/lib/gcc/x86_64-pc-linux-gnu/11.2.0/include/lwpintrin.h
         (parser state: 69; real character index: 364762):
  Expected a token of one of the following types:
   [edu:umn:cs:melt:ableC:concretesyntax:BlockComment_t,
    edu:umn:cs:melt:ableC:concretesyntax:Identifier_t,
    '(',
    edu:umn:cs:melt:ableC:concretesyntax:LineComment_t,
    edu:umn:cs:melt:ableC:concretesyntax:NewLine_t,
    edu:umn:cs:melt:ableC:concretesyntax:Spaces_t,
    edu:umn:cs:melt:ableC:concretesyntax:cppTags:CPP_Location_Tag2_t,
    edu:umn:cs:melt:ableC:concretesyntax:cppTags:CPP_Location_Tag_t,
    edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:PragmaMark]
   Input currently matches:
   [edu:umn:cs:melt:ableC:concretesyntax:gcc_exts:AttributeNameUnfetterdByKeywords_t,
    '__attribute__']

The code for it:

extern __inline void __attribute__((__gnu_inline__, __always_inline__, __artificial__))
__llwpcb (void *__pcbAddress)
{
  __builtin_ia32_llwpcb (__pcbAddress);
}

extern __inline void * __attribute__((__gnu_inline__, __always_inline__, __artificial__))
__slwpcb (void)
{
  return __builtin_ia32_slwpcb ();
}

Line 43 is the second definition. Column 23 is the __attribute__. My guess is it isn't liking the pointer * before that, since it was fine with the previous definition in the file.

@krame505
Copy link
Member Author

krame505 commented Apr 8, 2022

I seem to have sorted out the concrete syntax grammar (I think it's as simple as that attributes can appear anywhere that a list of qualifiers can) but ableC's abstract syntax doesn't currently support attributes in every place that qualifiers can appear. This is a bit annoying, but should be possible to address.

@remexre remexre mentioned this pull request Apr 28, 2022
@remexre
Copy link
Member

remexre commented Jun 9, 2022

I'm gonna un-mark myself as reviewer; I've been getting thrice daily slack pings abt this for a few months :P

@remexre remexre removed their request for review June 9, 2022 19:00
@remexre
Copy link
Member

remexre commented Oct 7, 2022

wrt last two commits, how bad was it?

@krame505
Copy link
Member Author

krame505 commented Oct 7, 2022

SIlver needed 7G of heap to run the flow analysis on just ableC by itself, the extensions would all presumably need even more.

Only the first commit was actually needed though, the second was me trying to diagnose the test failures.

…lling in all the xmm intrinsic crud."

This reverts commit 3a17801.
@krame505
Copy link
Member Author

krame505 commented Oct 7, 2022

I reverted the second commit. It seems the jenkins failures were because of @remexre's commit 3a17801 switching to use clang for C preprocessing, for some reason?

If this build passes, then I think this is good to review/merge. Probably good to get this in before we create a bunch of merge conflicts by removing autocopy.

@krame505 krame505 requested a review from remexre October 10, 2022 22:44
@krame505 krame505 merged commit 4fa299f into develop Oct 21, 2022
@krame505 krame505 deleted the fix/struct-attributes branch October 21, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants