Skip to content

Parse bounds expressions in function parameter list scopes. #17

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

Merged
merged 7 commits into from
Jul 12, 2016

Conversation

dtarditi
Copy link
Member

@dtarditi dtarditi commented Jul 8, 2016

This change addresses issue #7 (process bounds expressions for parameters
with all the parameters available). Currently, bounds expressions for
parameters are being parsed and type checked in the scope of the parameters
seen so far in a parameter list. They need to be typechecked in a scope with
all the parameters for the parameter list.

To do this, parsing and type checking of bounds expressions for parameters
is deferred until the entire parameter list has been seen. This is done
in two steps. During the initial parse, the tokens for a superset of bounds
expressions are parsed and stored in a list. They are then re-parsed using
ParseBoundsExpression after all parameters have been seen.

This change also improves error messages when a bounds expression is parsed
with misspelled contextual keyword. For example, int x : boounds(x, x + 5).
Before, the code would simply give up after parsing the misspelled keyword
and the '('. This led to additional error messages when the code tried to
match the closing ')'. This fix is to skip all the tokens up to but not
including the closing ')'.

Testing:

  • Passes existing clang test suite.
  • Added additional tests for parsing of function parameter lists with bounds
    expressions. This will be committed separately to the Checked C repo.

dtarditi added 6 commits June 30, 2016 19:40
This change only allows the -fcheckedc-extension flag to be used for C programs
in clang.  The clang driver will reject the use of -fcheckedc-extension for
other C family languages supported by clang, including C++, Objective
C, OpenCL, and CUDA. This addresses issue checkedc#9 in the checked-clang Github repo.

We are currently not modifying clang to support these other languages, which
is why need to disallow using the extension with them.

Testing:
-  Add 4 new tests to clang.  They test that use of the extension flag is
rejected for C++, Objective C, CUDA, and OpenCL.
- As recommended by the clang documentation, I placed the tests in with
  other similar tests in the test tree.
- I updated the testing baselines to reflect the new tests.  I also updated
the documentation to reflect the fact that we have Checked C specific tests
in clang. We need to take the new tests into account when updating to new
versions of the clang/LLVM sources.
Modify the new C test to use Checked C syntax for an
argument type.  Compile the test using clang-cl.
This change addresses issue checkedc#7 (process bounds expressions for parameters
with all the parameters available).  Currently, bounds expressions for
parameters are being parsed and type checked in the scope of the parameters
seen so far in a parameter list.  They need to be typechecked in a scope with
all the parameters for the parameter list.

To do this, parsing and type checking of bounds expressios for parameters
is deferred until the entire parameter list has been seen.  This is done
in two steps.  During the initial parse, the tokens for a superset of bounds
expressions are parsed and stored in a list.  They are then re-parsed using
ParseBoundsExpressions after all parameters have been seen.

This change also improve serror messages when a bounds expression is parsed
with misspelled contextual keyword.  For example, int x : boounds(x, x + 5).
Before, the code would simply give up after parsing the misspelled keyword
and the '('.  This led to additional error messages when the code tried to
 match the closing ')'.  This fix is to skip all the tokens up to but not
including the closing ')'.

Testing:
* Passes existing clang test suite.
* Added additional tests for parsing of function parameter lists with bounds
  expressions.  This will be commited separtely to the checked repo.
@msftclas
Copy link

msftclas commented Jul 8, 2016

Hi @dtarditi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (David Tarditi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@awruef
Copy link
Collaborator

awruef commented Jul 11, 2016

Looks good to me.

// bounds expressions for parameters is done in a scope that includes all
// the parameters in the parameter list.
typedef std::pair<ParmVarDecl *, CachedTokens *> BoundsExprInfo;
SmallVector<BoundsExprInfo, 4> deferredBoundsExpressions;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 [](start = 30, length = 1)

Is there specific thinking behind the selection of 4 here? It would be helpful to understand if there's something special about that value or if it was a reasonable guess at likely length...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a guess that most functions take 4 parameters or less. SmallVector<T,n> doesn't heap-allocate unless you add more than n elements. I'll add a comment about the magic constant.

@reubeno
Copy link

reubeno commented Jul 11, 2016

Apart from my questions above, changes generally look good.

@dtarditi dtarditi merged commit 4ca4bb4 into checkedc:master Jul 12, 2016
@dtarditi dtarditi deleted the issue7 branch July 29, 2016 20:16
mgrang pushed a commit that referenced this pull request Sep 27, 2019
Cherry-picked from commit 00143b7b0c010e852161ac8a9f1ee785effc3d26
kkjeer pushed a commit that referenced this pull request Sep 23, 2020
When `Target::GetEntryPointAddress()` calls `exe_module->GetObjectFile()->GetEntryPointAddress()`, and the returned
`entry_addr` is valid, it can immediately be returned.

However, just before that, an `llvm::Error` value has been setup, but in this case it is not consumed before returning, like is done further below in the function.

In https://bugs.freebsd.org/248745 we got a bug report for this, where a very simple test case aborts and dumps core:

```
* thread #1, name = 'testcase', stop reason = breakpoint 1.1
    frame #0: 0x00000000002018d4 testcase`main(argc=1, argv=0x00007fffffffea18) at testcase.c:3:5
   1	int main(int argc, char *argv[])
   2	{
-> 3	    return 0;
   4	}
(lldb) p argc
Program aborted due to an unhandled Error:
Error value was Success. (Note: Success values must still be checked prior to being destroyed).

Thread 1 received signal SIGABRT, Aborted.
thr_kill () at thr_kill.S:3
3	thr_kill.S: No such file or directory.
(gdb) bt
#0  thr_kill () at thr_kill.S:3
#1  0x00000008049a0004 in __raise (s=6) at /usr/src/lib/libc/gen/raise.c:52
#2  0x0000000804916229 in abort () at /usr/src/lib/libc/stdlib/abort.c:67
#3  0x000000000451b5f5 in fatalUncheckedError () at /usr/src/contrib/llvm-project/llvm/lib/Support/Error.cpp:112
#4  0x00000000019cf008 in GetEntryPointAddress () at /usr/src/contrib/llvm-project/llvm/include/llvm/Support/Error.h:267
#5  0x0000000001bccbd8 in ConstructorSetup () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:67
#6  0x0000000001bcd2c0 in ThreadPlanCallFunction () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:114
#7  0x00000000020076d4 in InferiorCallMmap () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:97
#8  0x0000000001f4be33 in DoAllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:604
#9  0x0000000001fe51b9 in AllocatePage () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:347
#10 0x0000000001fe5385 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:383
#11 0x0000000001974da2 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2301
#12 CanJIT () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2331
#13 0x0000000001a1bf3d in Evaluate () at /usr/src/contrib/llvm-project/lldb/source/Expression/UserExpression.cpp:190
#14 0x00000000019ce7a2 in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Target/Target.cpp:2372
#15 0x0000000001ad784c in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:414
#16 0x0000000001ad86ae in DoExecute () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:646
#17 0x0000000001a5e3ed in Execute () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandObject.cpp:1003
#18 0x0000000001a6c4a3 in HandleCommand () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:1762
#19 0x0000000001a6f98c in IOHandlerInputComplete () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2760
#20 0x0000000001a90b08 in Run () at /usr/src/contrib/llvm-project/lldb/source/Core/IOHandler.cpp:548
#21 0x00000000019a6c6a in ExecuteIOHandlers () at /usr/src/contrib/llvm-project/lldb/source/Core/Debugger.cpp:903
#22 0x0000000001a70337 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2946
#23 0x0000000001d9d812 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/API/SBDebugger.cpp:1169
#24 0x0000000001918be8 in MainLoop () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:675
#25 0x000000000191a114 in main () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:890```

Fix the incorrect error catch by only instantiating an `Error` object if it is necessary.

Reviewed By: JDevlieghere

Differential Revision: https://reviews.llvm.org/D86355

(cherry picked from commit 1ce07cd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants