Skip to content

Further improvements to installation of Checked C system headers #296

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

Closed
mattmccutchen-cci opened this issue Oct 21, 2020 · 8 comments
Closed
Assignees
Labels
enhancement New feature or request Upstream Work item that has to deal with making changes to upstream.

Comments

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Oct 21, 2020

Kyle pointed out that installing the Checked C system headers in order to run the Checked C compiler (or even 3C? I presume it uses the annotated declarations to initialize its constraint solving) is manual and a bit of a pain. We should be able to do better for checkedc-clang developers. We could do something just for our repo for now or go ahead and take up the issue with Microsoft (especially because we plan to send a PR of our whole repo to Microsoft soon, so anything we did in our repo would get sent).

For end users of Checked C / 3C (as contrasted with developers like us), Microsoft says the headers "are already on your clang system include path"; I haven't researched yet how they achieve that.

@mattmccutchen-cci
Copy link
Member Author

mattmccutchen-cci commented Oct 27, 2020

It sounds like we are supposed to git clone https://github.com/microsoft/checkedc into the llvm/projects/checkedc-wrapper directory, and then building clang should do the right thing automatically: see this documentation. (Note: As of 2020-10-27, https://github.com/correctcomputation/checkedc exists, so we should likely use it instead. TBD how this interacts with any of the solutions below.)

According to the documentation, Microsoft didn't include the Checked C system headers in the checkedc-clang repo because of a licensing concern. But since all the LLVM licenses are non-copyleft, I see no problem with including the headers under their existing license as long as it is clearly marked. So ideally, we can convince Microsoft to move the headers (and maybe some other stuff?) from microsoft/checkedc to microsoft/checkedc-clang and save all Checked C users the trouble. If not, other possibilities (depending on whether we can assume all microsoft/checkedc-clang users want the headers or there is some use case for not having them) include a git submodule, making the build system automatically clone the repo, or simply making the documentation of how to clone the repo more prominent.

Adding a related issue: it would be great if users didn't have to modify all their includes to reference the _checked.h headers. Mike said this is this way because if the Checked C headers have the original names, we haven't found a way for them to include the original system headers. But clang supports an #include_next directive that is supposed to do exactly this, and Checked C can be used only via clang, so it should be fine to use #include_next. Hopefully we can send a PR for this to Microsoft after checking for any previous discussion of the issue.

@kyleheadley
Copy link
Member

In my trials, cloning checkedc into checkedc-wrapper helped a lot. I no longer need to set CPATH.

@mwhicks1
Copy link
Member

mwhicks1 commented Dec 8, 2020

I think this issue is closed, now that the docs tell you how/where to install the headers.

@mwhicks1 mwhicks1 closed this as completed Dec 8, 2020
@mattmccutchen-cci mattmccutchen-cci added enhancement New feature or request Upstream Work item that has to deal with making changes to upstream. labels Dec 8, 2020
@mattmccutchen-cci mattmccutchen-cci changed the title Better process to install Checked C system headers Further improvements to installation of Checked C system headers Dec 8, 2020
@mattmccutchen-cci
Copy link
Member Author

We've made it a lot better, but as stated above, as a low priority, I'd still like to convince Microsoft to (1) just ship the headers in the same repository and (2) migrate to #include_next so we don't need update-includes.py any more.

@mwhicks1
Copy link
Member

mwhicks1 commented Dec 8, 2020

But these are changes that Microsoft would have to make, not us, right? So then we should submit issues to their repository.

@mattmccutchen-cci
Copy link
Member Author

mattmccutchen-cci commented Dec 8, 2020

Yes, this is a meta-issue asking someone (presumably me, if I get the time) to submit issues to Microsoft after checking for previous discussion on their side and gathering all the information they'll need (the general expectations before submitting issues to any repository). One of us could then consider putting in the time to submit a PR. #317 was another meta-issue until Microsoft went ahead and fixed the problem themselves (in fact, it appears they were encouraged to do so by the link to our meta-issue that they saw in our documentation), leaving us only the "issue" portion of testing their fix and updating our own documentation. In general, is it OK to file meta-issues (or in this case, let the meta-issue portion of a previous issue remain open) so we can record and discuss things ourselves and defer the work of researching them to send to Microsoft? It looks like we've created a few meta-issues with the "Upstream" label in the past.

This particular meta-issue affects all Checked C users equally and has no coupling to 3C. I know there have been several such issues in the past in which you've asked me not to get involved. But I think getting involved at some point may be the right choice if the benefit to us is significant (in this case we'd be able to remove several steps from our documentation), I already have a good understanding of the solution, and no one else seems likely to step up. Does that make sense? We can still defer the decision of whether I work on this particular meta-issue.

@mwhicks1
Copy link
Member

mwhicks1 commented Dec 9, 2020

For such issues that are not 3C-specific, I'm happy for you to submit an issue on the Microsoft site, but then let it go at that point. OTOH, if the fix for the issue is not time consuming (perhaps a couple of hours), that's fine to do. Note that even a couple of hours is a fair percentage of your weekly workload, and I'd prefer you spend time on 3C-specific work.

@mattmccutchen-cci
Copy link
Member Author

For such issues that are not 3C-specific, I'm happy for you to submit an issue on the Microsoft site, but then let it go at that point. OTOH, if the fix for the issue is not time consuming (perhaps a couple of hours), that's fine to do. Note that even a couple of hours is a fair percentage of your weekly workload, and I'd prefer you spend time on 3C-specific work.

Understood. For now, I'll plan to check with you before starting on any individual PR. I'd hope neither of these PRs would take more than a couple of hours, but I know I often fail to foresee complications. I wanted to make Microsoft aware of the possible enhancements and hopefully get their agreement in principle, but I'm fine to wait a while to implement them and get other stuff done first.

I went ahead and banged out both issues to Microsoft in about an hour: checkedc#957 and checkedc/checkedc#431. I think we can close this now and reopen it later if we find we have to do work on our side to adapt to the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Upstream Work item that has to deal with making changes to upstream.
Projects
None yet
Development

No branches or pull requests

3 participants