search

melt-umn / ableC

Attribute grammar Based Language Extensions for C
http://melt.cs.umn.edu/ableC/
GNU Lesser General Public License v3.0
37 stars 6 forks source link

Parameterized gcc Attributes are pretty-printed with erroneous parentheses #47

Open 200sc opened 7 years ago

200sc commented 7 years ago

An attribute like __attribute__(name(arg1, arg2)) is currently being written by ableC as __attribute__((name(arg1,(arg2)))), but this is invalid syntax.

The specific example that brought this up was posix_memalign on osx in stdlib.h. Including this will raise this error (as in the matlab extension):

simple_cat.c:838:95: error: expected 'introduced', 'deprecated', or 'obsoleted'
signed int posix_memalign(void  * * , size_t  , size_t  ) __attribute__((availability(macosx, ((introduced) = 10.6))));

Removing the extra parentheses (after availability) will let gcc run the file.

Ideally we'd catch something like this in the future by adding in some tests that run on or simulate an osx environment.

This TODO: mentioned in VariableAttributes.sv looks like it could solve this bug.

remexre commented 7 years ago

The cause looks to be

https://github.com/melt-umn/ableC/blob/develop/edu.umn.cs.melt.ableC/abstractsyntax/Expr.sv#L46

abstract production declRefExpr
top::Expr ::= id::Name
{
  ...
  top.pp = parens( id.pp );
  ...
}

We parenthesize all expressions except literals (as far as I can tell from quickly skimming) to avoid implementing precedence, so we could implement that to fix this.

Alternatively, Patrick is writing a helper that maps over the ExprList and returns declRefExpr's id's pp instead of its own, which should also fix the case where an attribute requires an identifier (rather than an expression). Literals don't get parenthesized either, so they should be fine as well.

Lastly, we could make declRefExpr's pp always be id.pp without parentheses -- I tried that, but I got every single positive test failing, so I assume something's wrong in my setup. (Or my running of the tests; it's testing/runTests, right?)

200sc commented 7 years ago

We tested on a mac if availability(macosx, (introduced = 10.6)) would run, and it did not, meaning that just removing parens from declRefExpr wouldn't be a fix, unfortunately.

krame505 commented 7 years ago

I don't know why we wrap declRefExpr in parens- @tedinski?

The testing directory in ableC is for interference testing, which has been broken for a while. To actually check if this breaks anything, push this in a new feature branch and Jenkins will let you know if there are problems.

Using the correct precedence for pp would be nice to have, but this is probably lower on out list of priorities at the moment.

On May 31, 2017 4:37 PM, "Patrick Stephen" notifications@github.com wrote:

We tested on a mac if availability(macosx, (introduced = 10.6)) would run, and it did not, meaning that just removing parens from declRefExpr wouldn't be a fix, unfortunately.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305325210, or mute the thread https://github.com/notifications/unsubscribe-auth/AIE1ivpWOU6P5aa1UPHqGExk9BLPYH1Xks5r_d2zgaJpZM4NsCF0 .

ericvanwyk commented 7 years ago

I'm not sure if ableC has ever worked on my Mac. It would be nice if it did, but this is not, in my opinion, a high priority.

I do plan on buying a Mac for my office that will be set up so that others can login to (or is it "log into"?) to let us eventually fix this.

On Wed, May 31, 2017 at 9:19 PM, Lucas Kramer notifications@github.com wrote:

I don't know why we wrap declRefExpr in parens- @tedinski?

The testing directory in ableC is for interference testing, which has been broken for a while. To actually check if this breaks anything, push this in a new feature branch and Jenkins will let you know if there are problems.

Using the correct precedence for pp would be nice to have, but this is probably lower on out list of priorities at the moment.

On May 31, 2017 4:37 PM, "Patrick Stephen" notifications@github.com wrote:

We tested on a mac if availability(macosx, (introduced = 10.6)) would run, and it did not, meaning that just removing parens from declRefExpr wouldn't be a fix, unfortunately.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305325210, or mute the thread https://github.com/notifications/unsubscribe-auth/ AIE1ivpWOU6P5aa1UPHqGExk9BLPYH1Xks5r_d2zgaJpZM4NsCF0

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305370480, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhdwkj5pp-CJOIE916y1b_e4uCe2Eiks5r_h_GgaJpZM4NsCF0 .

tedinski commented 7 years ago

I don't really know, sorry.

200sc commented 7 years ago

I can't find an attribute that isn't obscure-platform specific outside of clang's attributes that ableC currently fails to parse, so its fair to call this a mac issue and wait on it.

krame505 commented 7 years ago

If removing the parentheses from declRefExpr doesn't seems to break anything, I would say go ahead and do that.

On Jun 1, 2017 1:17 PM, "Patrick Stephen" notifications@github.com wrote:

I can't find an attribute that isn't obscure-platform specific outside of clang's attributes that ableC currently fails to parse, so its fair to call this a mac issue and wait on it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305576424, or mute the thread https://github.com/notifications/unsubscribe-auth/AIE1itN806i19oazH0wwWoiX51v_BQm0ks5r_wA_gaJpZM4NsCF0 .

remexre commented 7 years ago

It doesn't actually fix this issue though -- there's still extra parens around the assignment.

On Thu, Jun 1, 2017, 13:23 Lucas Kramer notifications@github.com wrote:

If removing the parentheses from declRefExpr doesn't seems to break anything, I would say go ahead and do that.

On Jun 1, 2017 1:17 PM, "Patrick Stephen" notifications@github.com wrote:

I can't find an attribute that isn't obscure-platform specific outside of clang's attributes that ableC currently fails to parse, so its fair to call this a mac issue and wait on it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305576424, or mute the thread < https://github.com/notifications/unsubscribe-auth/AIE1itN806i19oazH0wwWoiX51v_BQm0ks5r_wA_gaJpZM4NsCF0

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305578077, or mute the thread https://github.com/notifications/unsubscribe-auth/AEAJtbyFUlBnA-ZUOJbWvEov6vAsd-Pbks5r_wGogaJpZM4NsCF0 .

krame505 commented 7 years ago

Yeah, handling this specially might be the best option... Currently the halide extension is using some hacks to get around this issue when generating omp pragmas.

On Jun 1, 2017 1:32 PM, "Nathaniel Ringo" notifications@github.com wrote:

It doesn't actually fix this issue though -- there's still extra parens around the assignment.

On Thu, Jun 1, 2017, 13:23 Lucas Kramer notifications@github.com wrote:

If removing the parentheses from declRefExpr doesn't seems to break anything, I would say go ahead and do that.

On Jun 1, 2017 1:17 PM, "Patrick Stephen" notifications@github.com wrote:

I can't find an attribute that isn't obscure-platform specific outside of clang's attributes that ableC currently fails to parse, so its fair to call this a mac issue and wait on it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305576424, or mute the thread < https://github.com/notifications/unsubscribe-auth/ AIE1itN806i19oazH0wwWoiX51v_BQm0ks5r_wA_gaJpZM4NsCF0

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305578077, or mute the thread https://github.com/notifications/unsubscribe-auth/AEAJtbyFUlBnA- ZUOJbWvEov6vAsd-Pbks5r_wGogaJpZM4NsCF0 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/47#issuecomment-305580757, or mute the thread https://github.com/notifications/unsubscribe-auth/AIE1ihdRkWqhEy4GEF6EDaboCkuIKiHQks5r_wPXgaJpZM4NsCF0 .