Skip to content

Parsing of member bounds declarations. #23

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 3 commits into from
Aug 2, 2016
Merged

Conversation

dtarditi
Copy link
Member

@dtarditi dtarditi commented Jul 29, 2016

This change adds parsing of member bounds declarations (issue #19). Member bounds
declarations use member bounds expressions, which can only use other
members of the structure. The changes include:

  • IR: no change is needed to represent member bounds expressions. Because
    clang handles C++, it already can represent members directly in
    the IR for expressions. Bounds expressions are a subclass of expressions.

    We need to attach bounds expressions to member declarations. We already
    have bounds expressions on function declarations and variable declarations,
    which have the same base class as member declarations. Instead of having
    a 3rd copy of the code, we factor the common code for representing bounds
    expressions and move it to the common base.

  • Parsing: we modify the parsing of member declarators that are followed
    by ':' to parse either a bounds expression or a constant-expression
    that is a bit-field size. We check whether the first token after the
    ':' is an identifier that is contextual keyword for bounds expressions.

    The parsing of bounds expressions is deferred until all members of the
    structure have been seen. This is so that member bounds expressions may
    refer to any member of the structure. Constant expressions for bitfield
    sizes continue to be eagerly parsed.

    The decision to just look for a bounds expression keyword can lead
    to several error messages from clang if there is a spelling
    error in a contextual keyword. Clang interprets the misspelled keyword
    as an implicit function declaration and issues unknown identifier errors
    for uses of members.

    For now, we'll live this because this specific case is a fairly
    low priority to address. The type for the identifier in the declarator
    hasn't been built yet at the time that we make the parsing decision.
    We could move that up or defer parsing of constant expressions. Either
    change is largely orthogonal to this change.

  • Semantic processing: member bounds expressions are restricted to accessing
    only members. We add a state flag to Sema that changes the lookup of C
    identifiers during expression parsing to look up identifiers in the
    member namespace instead of the ordinary namespace. The state flag is
    protected by an RAII class.

Testing:

  • Wrote new feature tests of parsing member bounds
    declaration. These will be checked in separately to the Checked C
    repo in parsing\member_bounds.c.
  • Passes existing clang test baseline.

This change adds parsing of member bounds declarations.  Member bounds
decclarations use member bounds expressions, which can only use other
members of the structure.  The changes include:
- IR: no change is needed to represent member bounds expressions. Because
  clang handles C++, it already can represent members directly in
  the IR for expressions.  Bounds expressions are a subclass of expressions.

  We need to attach bounds expressions to member declarations.  We already
  have bounds expressions on function declarations and variable declarations,
  which have the same base class as member declarations.  Instead of having
  a 3rd copy of the code, we factor the common code for representing bounds
  expressions and move it to the common base.

- Parsing: we modify the parsing of member declarators that are followed
  by ':' to parse either a bounds expression or a constant-expression
  that is a bit-field size. We check whether the first token after the
  ':' is an identifier that is contextual keyword for bounds expressions.

  The parsing of bounds expressions is deferred until all members of the
  structure have been seen.  This is so that member bounds expressions may
  refer to any member of the structure.   Constant expressions for bitfield
  sizes continue to be eagerly parsed.

  The decision to just look for a bounds expression keyword can lead
  to several error error messages from clang if there is a spelling
  error in a contextual keyword.   Clang interprets the misspelled keyword
  as an implicit function declaration and issues unknown identifier errors
  for uses of members.

  For now, we'll live this because this specific case is a fairly
  low priority to address.  The type for the identifier in the declarator
  hasn't been built yet at the time that we make the parsing decision.
  We could move that up or defer parsing of constant expressions.  Either
  change seems likely orthogonal to this change.

- Semantic processing: member bounds expressions are restricted to accessing
only members.  We add a state flag to Sema that changes the lookup of C
identifiers during expression parsing to look up identifiers in the
member namespace instead of the ordinary namespace.  The state flag is
protected by an RAII class.

Testing:
- Wrote a new feature tests of parsing member bounds
  declaration.  These wil be checked in separately in the Checked C
  repo in parsing\member_bounds.c.

- Passes existing clang test baseline.
@msftclas
Copy link

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;

@@ -1584,8 +1582,6 @@ class FunctionDecl : public DeclaratorDecl, public DeclContext,
/// 'enum Y' in 'void f(enum Y {AA} x) {}'.
ArrayRef<NamedDecl *> DeclsInPrototypeScope;

BoundsExpr *ReturnBoundsExpr;
Copy link

@reubeno reubeno Jul 29, 2016

Choose a reason for hiding this comment

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

ReturnBoundsExpr [](start = 14, length = 16)

I'm not sure where it would go, but it might be helpful to have a comment somewhere explain the intended meaning of 'Bounds' for FunctionDecls--that it's intended to describe the return value of the function. #Resolved

Copy link
Member Author

@dtarditi dtarditi Aug 1, 2016

Choose a reason for hiding this comment

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

There are already comments on the getter functions in the superclass of FunctionDecl. These will be picked up the automatic doc generation and by IDE tool tips (for IDEs that understand /brief comments). I've add a comment to the setter function too. #Resolved

@reubeno
Copy link

reubeno commented Jul 30, 2016

This generally looks good to me--just had a few low-priority suggestions around the code and a few questions around the lifetime of cached tokens.

- Track CachedTokens for deferred bounds expressions using unique_ptr, not *,
  so that it is clear that it is freed.
- Remove unnecessary dynamic cast to DeclaratorDecl.
- Remove stray spaces and newlines.
- Make sure it is clear in comments that the bounds expression for a Function
  Decl is the return bounds expression.
- Create a function for detecting that a token is the start of a bounds
  expression.  This centralizes the code in ParseExpr.cpp.
- Add a comment for StartBoundsExpression
- Remove a stray space.
@dtarditi dtarditi merged commit 3e625d9 into checkedc:master Aug 2, 2016
@dtarditi dtarditi deleted the issue19 branch August 12, 2016 00:04
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