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

Highlevel codegen #2498

Open
wants to merge 80 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kodebach
Copy link
Contributor

kodebach commented Mar 14, 2019

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

Review

@markus2330 This PR is technically still WIP, but apart from documentation and minor changes (among others integration of the options plugin) everything is finished.

For some examples take a look at tests/shell/check_gen.sh and the test data in tests/shell/gen/elektra.

Other notes

For now I just (rather crudely) disabled the stuff related to the old kdb gen. The reason I didn't remove it is, that the new kdb gen doesn't support the contextual values part of the old version. It is possible to support it as is, but I think there may be a better solution involving less template magic and more code generation.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 15, 2019

Thank you for creating the PR!

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 15, 2019

If anybody has input as to why as to why the mac build jobs seem to have problems with inttypes.h (@sanssecours ?), I would be very thankful.

@sanssecours

This comment has been minimized.

Copy link
Member

sanssecours commented Mar 16, 2019

If anybody has input as to why as to why the mac build jobs seem to have problems with inttypes.h, I would be very thankful.

Honestly, I do not know why the test fails. It does works on my machine. Maybe there is something wrong with the Xcode installation on the Cirrus CI macOS images? My suggestions would be to

.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 17, 2019

@markus2330 do you have any ideas how to get rid of the ubsan problems with the ni plugin?

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 22, 2019

Where do I see these problems? I only see a failing testscr_check_gen

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 22, 2019

Btw. we can rename the old gen to "python-gen"

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 22, 2019

Where do I see these problems? I only see a failing testscr_check_gen

Yes, and the test fails because there is a ubsan problem in the nickel library used by ni:

../src/plugins/ni/nickel-1.1.0/src/hash.c:451:6: runtime error: unsigned integer overflow: 2575944922 + 1852143981 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/plugins/ni/nickel-1.1.0/src/hash.c:451:6 in 

from https://travis-ci.org/ElektraInitiative/libelektra/jobs/507313602#L2692

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 22, 2019

Seems to be a valid complain. Why is this triggered now?

@KurtMi: can we maybe reuse our hashing function also there? This function is a pain to maintain.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 22, 2019

Why is this triggered now?

Probably, because this one path of the hash function using values like these never came up before...
But because it is a hash function it might actually be intended that the values wrap around, which for unsigned integers is the overflow behaviour specified by the standard since at least C99.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 23, 2019

I can only guess. Based on the code comments this second code path is actually only there to make sanitizers happy. So if @KurtMi does not have another idea, we should go for suppression.

@KurtMi

This comment has been minimized.

Copy link

KurtMi commented Mar 25, 2019

@KurtMi: can we maybe reuse our hashing function also there? This function is a pain to maintain.

I introduced the ELEKTRA_NO_SANITIZE_UNDEFINED and ELEKTRA_NO_SANITIZE_INTEGER macros see in opmphm.c line 647 for usage.

In the mentioned opmphm.c file is the hashlitte implemented, in the nickel 1.1 are more but I am not sure what functions nickel really uses.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 26, 2019

@KurtMi: So we can safely suppress these errors?

@KurtMi

This comment has been minimized.

Copy link

KurtMi commented Mar 26, 2019

@KurtMi: So we can safely suppress these errors?

Well, this is a hash function, I guess some overflows are wanted.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 26, 2019

@kodebach so simply suppress these errors with the macro ELEKTRA_NO_SANITIZE_INTEGER.

Or should we make a new macro ELEKTRA_NO_SANITIZE_UNSIGNED_INTEGER?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 26, 2019

We may also try this https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#silencing-unsigned-integer-overflow

But I am not sure how to enable this feature only for certain files or parts of files...

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 26, 2019

I think it is better to suppress too much locally than to suppress anything globally. Creating ELEKTRA_NO_SANITIZE_UNSIGNED_INTEGER shouldn't be too difficult, see 8987165

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 26, 2019

I think it is better to suppress too much locally than to suppress anything globally. Creating ELEKTRA_NO_SANITIZE_UNSIGNED_INTEGER shouldn't be too difficult, see 8987165

It would have to be ELEKTRA_NO_SANITIZE_UNSIGNED_INTEGER_OVERFLOW.
-fsanitize=unsigned-integer-overflow is part of the group -fsanitize=integer, but there is no group for unsigned integers

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks

@kodebach kodebach force-pushed the kodebach:highlevel_codegen branch 2 times, most recently from 46f1bc1 to bbcdad7 Mar 29, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Apr 4, 2019

@markus2330 IMO the only thing missing here are documentation and more automatic tests. Could you please check, if there are any major features missing before I do that? (so that I don't have to update the tests over and over again)

To see how the code-generation works and what its current features are, you can take a look at the two new examples. The READMEs should tell you how to use them. (Apart from the fact that, because of the spec plugin's problem, you may need to define a lot of keys that should have default values)

@@ -58,11 +58,11 @@ gen/enum/type = Charset

[format/#/trim]
type = boolean
default = true

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 4, 2019

Contributor

Is this not working?

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 4, 2019

Author Contributor

I didn't try it, the type plugin would have to be mounted of course. Whether that is automatically done by spec-mount I don't know.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 4, 2019

Contributor

Yes, spec-mount should mount the type plugin if a "type" specifier is present. But if the default is transformed, I do not know: the key is added by the spec plugin, is the type plugin then executed later?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Apr 10, 2019

@markus2330 is there any reason why kdbmacros.h isn't installed?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Apr 13, 2019

While improving/fixing my tests, I had many problems with the spec plugin. Mostly it just didn't do the globbing correctly and didn't copy the metadata as expected. (Don't know the issue number right now, but I am pretty sure there already is one...)

I also increasingly think that the spec plugin is just not powerful enough, and specifications have to be integrated deeper into Elektra. Take for example this simple specification:

[mykey/_]
default = 5

Since _ matches everything but array elements, one would expect that kdb get /.../mykey/test would return 5. But it doesn't. The spec plugin has no way of knowing about /.../mykey/test. It has no value and therefore doesn't exist in the KDB. I thought maybe calling kdb setmeta user/.../mykey/test dummy 123 might trigger the spec plugin to copy metadata, but it doesn't really. The result kdb get /.../mykey/test is an empty string in this case...

@markus2330 markus2330 referenced this pull request Apr 14, 2019

Open

spec: make more expressive #2632

0 of 3 tasks complete
@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 14, 2019

@markus2330 is there any reason why kdbmacros.h isn't installed?

No, I assume it was forgotten.

While improving/fixing my tests, I had many problems with the spec plugin. Mostly it just didn't do the globbing correctly and didn't copy the metadata as expected. (Don't know the issue number right now, but I am pretty sure there already is one...)

Please report. It is easier to mark issues as duplicate as to run into the same issue twice.

I also increasingly think that the spec plugin is just not powerful enough

Yes, I agree. Please create new issues for proposals/bugs. (What you described with kdbset sounds like a bug). I created a meta-issue for spec features in #2632.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Apr 14, 2019

What you described with kdbset sounds like a bug

I am not so sure... This part

I thought maybe calling kdb setmeta user/.../mykey/test dummy 123 might trigger the spec plugin to copy metadata, but it doesn't really.

probably is a bug. But the original problem is an intrinsic problem of the spec plugin. The globbing expressions are resolved in kdbGet. For arrays you know the size and can create keys for all elements, but for the _ glob you just can't do anything for keys that don't exist yet.

IMO the only real solution is to integrate the globbing functionality into (a version of) ksLookup. It is the only place, where you can detect that test/a should use the data from test/_ as a fallback.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 15, 2019

IMO the only real solution is to integrate the globbing functionality into (a version of) ksLookup

we already had a ksLookupRE which worked with regular expressions (it actually still exists as part of the validation plugin). Afaik it was never used by anyone.

But we will hopefully soon get a new query layer, let us see how this works.

@kodebach kodebach force-pushed the kodebach:highlevel_codegen branch from 80d4216 to 15f2e98 Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.