configure: checks for headers and -fno-rtti #489
Conversation
mobin-2008
added
A-Importance: Normal
make
Ok but (keep in mind in future that) a PR summary should be a summary of the changes in the PR. "Commits are self-explanatory" doesn't tell anybody anything. Of course the commit messages will cover what has changed in the PR. That is true for every PR. The PR summary should summarise the changes, give any additional information, specify what testing has been done, etc. Have these changes been tested? |
|
Also this really should have been at least 2 separate PRs. |
configure
Outdated
|
|
||
| # Test if the compiler compiles the passed source file with an optional argument; if so return | ||
| # success otherwise return failure. | ||
| # $1 - the name of the source file to compile (without .cc suffix) |
There was a problem hiding this comment.
Why without the .cc suffix? That seems strange to me
If the object file name was also specified as an argument, it would all be more straightforward.
There was a problem hiding this comment.
Why without the .cc suffix? That seems strange to me
Because not to have to use "${1%%.cc}.o" for object file name.
If the object file name was also specified as an argument, it would all be more straightforward.
Agree. Also fixes the need for "${1%%.cc}.o". I will change it to get the object file name from argument.
There was a problem hiding this comment.
Because not to have to use "${1%%.cc}.o" for object file name.
But you wouldn't have to. You could just use $1.o.
Anyway, better to pass the object name in explicitly.
configure
Outdated
| # CXXFLAGS, CXXFLAGS_EXTRA, LDFLAGS, LDFLAGS_EXTRA - any predetermined flags | ||
| try_compile_source() | ||
| { | ||
| eval $CXX $CXXFLAGS ${2:-} $CXXFLAGS_EXTRA $LDFLAGS $LDFLAGS_EXTRA '"$configtmpdir"/"$1.cc"' \ |
There was a problem hiding this comment.
Why does this have $LDFLAGS $LDFLAGS_EXTRA if it's not linking?
There was a problem hiding this comment.
This is because I didn't want to change the flags from try_cxx_argument(). That function also doesn't link but for some reason has $LDFLAGS $LDFLAGS_EXTRA. Should I change this?
There was a problem hiding this comment.
Seems like they were left from a while back, probably by accident. Yes, remove them please.
configure
Outdated
| return $? | ||
| } | ||
|
|
||
| # Test if the compiler compiles and links the passed source file then execute it; returns program |
There was a problem hiding this comment.
"Test if the compiler successfully compiles and links a source file, and execute it if so;"
configure
Outdated
| { | ||
| eval $CXX $CXXFLAGS ${2:-} $CXXFLAGS_EXTRA $LDFLAGS $LDFLAGS_EXTRA '"$configtmpdir"/"$1.cc"' \ | ||
| -c -o '"$configtmpdir"/"$1.o"' > /dev/null 2>&1 | ||
| return $? |
configure
Outdated
| cat << _EOF > "$configtmpdir/testheader.cc" | ||
| #include <$2> | ||
| int main() |
There was a problem hiding this comment.
Is the main function needed for some reason I can't think of?
There was a problem hiding this comment.
No. I will remove it.
configure
Outdated
| if try_compile_source testheader; then | ||
| sub_info "Yes." | ||
| rm "$configtmpdir"/testheader.o | ||
| eval "$1=\"1\"" |
There was a problem hiding this comment.
There doesn't need to quotes around 1
configure
Outdated
| eval "$1=\"1\"" | ||
| else | ||
| sub_info "No." | ||
| eval "$1=\"0\"" |
There was a problem hiding this comment.
There doesn't need to be quotes around 0
configure
Outdated
| #include <string> | ||
| #include <stdexcept> | ||
| int main() | ||
| { | ||
| try { | ||
| std::stoull("invalid for stoull"); | ||
| } | ||
| catch (std::invalid_argument &exc) { | ||
| return 0; | ||
| } | ||
| return -1; |
There was a problem hiding this comment.
Can this be moved out-of-line? It's awkward to have a here-document in the middle of code. Have a separate function to generate this file.
There was a problem hiding this comment.
Will create write_no_rtti_check() function for such purpose.
configure
Outdated
| try_optional_cxx_argument CXXFLAGS -fno-rtti | ||
|
|
||
| # -fno-rtti is a special case. Some compilers (e.g. clang+libcxxrt) generate broken code | ||
| # when disabling rtti with code which uses exceptions. We need to make sure -fno-rtti |
There was a problem hiding this comment.
rtti -> RTTI
Or even better, "run-time type information".
"generate broken code" ... "doesn't break exception handling" - this is saying the same thing twice but in two different ways, making it seem like there might be two different issues. Just say one thing.
There was a problem hiding this comment.
Will be changed to
# -fno-rtti is a special case. Some compilers (e.g. clang+libcxxrt) generate broken code
# when disabling Run-Time Type Information with code which uses exceptions.There was a problem hiding this comment.
I said "RTTI" or "run-time type information", but you changed it to "Run-Time Type Information" :(
Acronyms should be capitalised which is why it would be "RTTI" and not "rtti". But "run-time type information" has been expanded from the acronym.
|
Another issue I just thought of: this will break cross-builds, there is no fallback for those.
I still don't have an answer to this. |
Can we just skip RTTI check when
Changes are tested on my Linux system and a FreeBSD VM. ioprio.h header check is tested by removing header. I don't have hardware to test on Mac. |
Yes, and not compile with (edit): And, clearly document this, and issue a warning at configure time.
That's fine, as long as you have tested (with the negative case too). |
|
Also please revise the commit messages.
Both summaries are too long (>72 characters)., and are saying "add X and a function to do X" in the summary line (and have an empty body). Can you look at the existing commit log messages and read some guides on how to write good commit messages, please. The summary line should summarise the functional change. |
From
I thought the limit is 76 characters not 72. Am I missing something or this needs to be changed in the doc.
Ok. How about these: Is it better? I fixed the line width and also added additional context for ioprio.h header check. |
Ok, that's wrong, it is supposed to say 72 characters. But that's a generous maximum, most messages should be shorter than that. Most references suggest 50 characters for the first line.
It is better, but the first one: Lacks any context. Compare with 43c1a3e: While you don't need to reproduce all that, it should say something and at least link to the issue. Also it is missing "an" in front of "additional", but, it doesn't need the "additional" anyway. I guess what you meant was: But that it is putting the real change to the side. It should be focussing on the main thing, something like: The 2nd one (for the header check) is a bit better. But, it has some minor grammar mistakes that should be corrected (assume -> assumes, introduce a method -> introduces a method). I would word the summary line as: "Introduce" is just noise. Say what the change does, functionally, not what changes have been made to the code. In the body you can mention code changes (but they should still be secondary to explanation of the actual functional change). |
I think it's better to say "try limit your commit message length to 50, the absolute maximum is 72 characters" similar to how character limit for Dinit code works.
So like this: Removed
Like this: Removed |
Ok.
This is two sentences with no connection between them.
|
|
The only thing that I'm not sure about is syntax of # Test if the compiler successfully compiles and links a source file, and execute it if so; returns
# program exit code or failure on compile/link failure.
# $1 - the name of the source file to compile, link and execute (without .cc suffix)The code adds |
Yeah. "was disabled" is what I meant.
So something like this: |
It is ok, but "which introduces a method" is redundant. We now check for ioprio.h via check_header(), a new function |
Removed. |
Yes that sounds ok. |
|
Looks like all issues are resolved in my local repo, I will self-review the changes tomorrow and will push it to remote repo for your final review. |
mobin-2008
force-pushed
the
configure_checks
branch
from
50521b0 to
78c81a3
Compare
|
After self-review and testing on my Linux system (with negative case for |
Did you miss this? I don't see any documentation updates |
Don't write in a conversational style. ("I think ..."). Look at the style of existing commit messages, please. They are not for opinions, they are for describing the changes in the commit. The summary line doesn't describe the functional nature of the change. (What does it even mean to "add a keyword" to a function?). Summarise the functional nature of the change. If you can't explain the entire change in the summary line, then at least be clear on whether it is fixing something, improving something, refactoring, or whatever, and then give details in the message body. In addition, you are not giving enough detail about the change in the message. I know what that means, but only because I'm familiar with this code. Explain it properly so that someone else can read this commit message, and easily understand the change. As it is, it's raising more questions than it answers (how is "setting/unsetting a dummy variable" used to only add the flag to one of the compiler/link flags variables? how does "adding a dash keyword" help?). Also, a dash is not a keyword (it isn't a word). Basically: the summary line is way too specific and can't be understood without additional context (it should be able to stand by itself), whereas the message body isn't specific enough and lacks a detailed explanation (skips straight from 'we sometimes add to only compiler or link flags' - without explaining why, but saying it's "common" - to 'dash is more reasonable than setting/unsetting a variable for this purpose' - without first explaining that passing a dummy variable as an out-parameter is how we avoid changing the real compilation flags/link flags variable, and without explaining that a dash can now be passed in place of an out-parameter so that a dummy variable is no longer needed. There's no way anyone could understand this commit message without looking at the actual code change. |
Sorry about the delay. Forgot to add doc. Is this comment enough? ...
if [ -n "${CXX_FOR_BUILD:-}" ]; then
# May not be able to execute the check when cross-compiling.
sub_info Cross-compilation detected. Disabling -fno-rtti to be safe about exception handling.
else
... |
I rewrote the commit message: Removed mentions to "keyword" both in commit message and code. It now explains why we want writing only to one variable. It now explains how it was done before and shows how the dash replaced the old trick. |
It is a lot better, thanks. I may suggest some minor corrections on review, but in the meantime:
No, it needs documentation. Anyone building this with a cross-compile is going to need to know what "Cross-compiltion detected. Disabling -fno-rtti to be safe about exception handling." means. |
Something like this? I modified the |
No, that is buried in a section called "recommended compiler options" which is independent of how the build is configured. And, it doesn't include the warning message issued by configure, so anyone who searches for it won't find it. It's best to add a new section to the BUILD specifically for "configure", at this stage. |
|
I am going to review what you have now because I want to get this PR cleared. Please don't forget to add the documentation, and be careful to make it clear. |
There was a problem hiding this comment.
I think you need to take more time with writing code documentation comments.
The information in them is sometimes jumbled, and it's not always correct. Also, some of the functions are doing things that the function documentation doesn't say anything about, and it's not clear why.
Please don't rush when writing the comments, they are just as important as the code.
configure
Outdated
| -c -o '"$configtmpdir"/"$2"' > /dev/null 2>&1 | ||
| } | ||
|
|
||
| # Test if the linker successfully links the specified object file. If the optional link argument |
There was a problem hiding this comment.
Test if the specified object file can be linked into an executable.
This doesn't invoke the linker directly, you're invoking the compiler driver, which perhaps does invoke the linker if it accepts the arguments it's given. It's not just testing the linker.
configure
Outdated
| } | ||
|
|
||
| # Test if the linker successfully links the specified object file. If the optional link argument | ||
| # is specified ($3) then pass it to the linker along with the already-established arguments |
There was a problem hiding this comment.
"pass it to the linker" - no, you're not invoking the linker here.
The optional argument specified here isn't passed to the linker - do you understand the difference between a linker, compiler, and compiler driver?
There was a problem hiding this comment.
Yeah, that actually passes flags to the compiler driver. Changed to compiler driver.
There was a problem hiding this comment.
The (this can be used to test if the linker accepts the argument) should mention linker or compiler driver?
configure
Outdated
| # $2 - the name of the result executable file | ||
| # $3 - the argument to test (optional) | ||
| # CXX - the name of the compiler | ||
| # LDFLAGS, LDFLAGS_EXTRA - any predetermined flag |
There was a problem hiding this comment.
any predetermined flag_s_ for linking
configure
Outdated
| try_link_object() | ||
| { | ||
| if [ ! -e "$configtmpdir"/"$1" ]; then | ||
| # If the object file doesn't exist yet, need to compile |
There was a problem hiding this comment.
This behaviour isn't documented, why is testing whether the object already exists?
There was a problem hiding this comment.
This is not documented and also it's not really necessary, I mean, instead of guessing source file name when object file doesn't exist just error out and make it caller responsibility to make sure object file exists.
There was a problem hiding this comment.
Moved the mentioned check to try_ld_argument and now try_link_object is just a command (like try_compile_source).
configure
Outdated
| # $3 - the name of the result executable file | ||
| try_execute_source() | ||
| { | ||
| if [ ! -e "$configtmpdir"/"$3" ]; then |
There was a problem hiding this comment.
Why is it testing if the executable file already exists? This isn't documented in the function comment
There was a problem hiding this comment.
After thinking more about this, I decided to remove try_execute_source completely in favor of try_link_object no-rtti-check.o no-rtti-check && "$configtmpdir"/no-rtti-check. This is more useful.
configure
Outdated
| # Note: caller can specify the dash ("-") keyword instead of the compiler/link-flags variable name | ||
| # to avoid writing flag to the compiler/link-flags variable. |
There was a problem hiding this comment.
Don't use something like a "Note:" for something that is critical to understanding the behaviour of a function. A "note" is for additional information, clarification, etc.
This should be part of the argument description, or completely specify the behaviour. Right now this "note" doesn't make sense until you've read the argument descriptions, but those come afterwards. You either need to describe the relevant arguments here as well so that this makes sense, or move this information into the argument descriptions.
Don't present a mish-mash of info so that something which comes first doesn't make sense until you read something that comes after it.
There was a problem hiding this comment.
# Test if an argument is supported during both compiling and linking.
# $1 - the name of the compiler-flags variable to potentially add the argument to (or specify the
# dash ("-") to skip adding to the variable)
# $2 - the name of the link-flags variable to potentially add the argument to (or specify the
# dash ("-") to skip adding to the variable)
# $3 - the argument to test/add
# CXX - the name of the compiler
# CXXFLAGS, CXXFLAGS_EXTRA, LDFLAGS, LDFLAGS_EXTRA - any predetermined flags
I moved the dash description to argument description section.
configure
Outdated
| # when disabling run-time type information with code which uses exceptions. | ||
| info Checking whether -fno-rtti is accepted for compilation... | ||
| if [ -n "${CXX_FOR_BUILD:-}" ]; then | ||
| sub_info Cross-compiltion detected. Disabling -fno-rtti to be safe about exception handling. |
There was a problem hiding this comment.
Did you check spelling? - "compiltion"
And grammar? - "to be safe about exception handling" is not grammatically correct.
"Some compilers (e.g. clang+libcxxrt)" - are there any other compilers that you know of that this applies to?
There was a problem hiding this comment.
Did you check spelling? - "compiltion"
I don't how this wasn't fixed by my last push but it's correct in my local repo.
And grammar? - "to be safe about exception handling" is not grammatically correct.
I think this is better and I don't see any grammar issue in it:
Cross-compilation detected. Disabling -fno-rtti to make sure exceptions will always work.
"Some compilers (e.g. clang+libcxxrt)" - are there any other compilers that you know of that this applies to?
No. Fixed to only mention clang+libcxxrt as known case:
...
# -fno-rtti is a special case. Clang+libcxxrt is known for generating broken code
# when disabling run-time type information with code which uses exceptions.
...
I have some changes in my local branch (discussed about them above). I will add the documentation and fixes requested in the next push. (I'm really busy and tired this week, I would work on this on Wednesday). |
|
Hi Mobin, what's the status of this PR? |
Hi. I'm going to work on it today. |
This makes configure more flexible for other checks such as header checks and more.
It's common to check a flag against both the compiler and linker, but
only add the flag to one of the compiler or link flags variables to
avoid duplicate flags. It was done by specifying a "dummy" variable
which the caller would then unset.
The dash ("-") can now be specified instead of a variable name for this
purpose.
SUPPORT_IOPRIO was disabled by default on Linux because configure assumes ioprio.h is not available on the system. We now check for ioprio.h via check_header(), a new function for checking header usability. See davmac314#478
There is an issue with catching standard library exceptions when using -fno-rtti with some combination of clang and runtime libraries. See davmac314#486
|
For the documentation, I thought of two options:
I already have this: I can add this (with some modification e.g. better grammar and adding the exact warning from configure) or write an entire new section just for configure to cover this up as you suggested. @davmac314 WDYT? |
It would be fine to have two different sections, one specifically about using "configure" and another one detailing the Clang+libcxxrt problem. Then in the "configure" section you can discuss how it's not always possible to auto-detect whether compiler options function as desired, and reference to the other section as one such case. If you mention the warning from "configure" (and I think you should), you would do that in this section since it is about using configure. You can't just have a section called "Special note for run-time type information" and expect people to read it if they don't already know what "run time type information is", especially if they're looking for information about a message which doesn't say anything about "run time type identification" (it mentions See the existing section with a similar title that you probably modeled this on:
That's also about an issue that breaks exception handling, but notice how the title doesn't mention exception handling - it mentions the important thing from the user perspective that indicates whether they may need to read this section. Do you see the difference, and understand why this is more useful? This part: ... is hard to make sense of. You first say that RTTI is "useful" (don't you mean necessary?) for I mentioned this in last review: there aren't any other compilers which are known to have this problem. If you say "some compilers" and then call out Clang+libcxxrt specifically, it implies that there are others as well. (Some others might but we don't know that, do we?). The wording "break exceptions" is very vague. There's no reason to be this vague. Even if it's just "break handling of certain exceptions" it would be a lot more precise. "Issue" and "bug" should be the other way around - it's the issue regarding the bug that can be found at the URL (look at the URL! It's even got Finally,
Information about a warning from configure doesn't belong in a section about some compiler option, it belongs in a section about using configure. I thought I had been clear about this. |
|
I really think you need to plan out what you are going to write before you write it. In this case a good plan might look like:
You need to set things out so you can write in a logical way. Otherwise you are missing details and jumbling things up. I really think you should take this approach when writing documentation: first, write a plan of what you want to say - as dot points like above, making sure each point has a specific focus and that you don't introduce more than one topic per point; and 2nd, expand it out to complete text. |
It was a little outdated and unclear. For instance, we have a confirmed case of a Dinit crash on FreeBSD with the newest Clang+libcxxrt when using -fno-rtti (davmac314#486), but the doc said "(and historically FreeBSD, IIRC)" had the exception handling problem with -fno-rtti.
The reference to configure is now refactored into its own section with more information about it. This is done because of new checks in configure.
mobin-2008
force-pushed
the
configure_checks
branch
from
78c81a3 to
6059416
Compare
|
Added two commits for two new sections in |
|
Please answer questions from the review before pushing changes.
This wasn't addressed in the changes.
This wasn't answered either. In the changes you just pushed you expanded it to Please answer questions in the review before pushing changes. That's part of the documented process. I was hoping to do a release soon and this is just wasting time. I realise that I had some rhetorical questions in the review, which I should try to avoid. But those questions above were not rhetorical, I want answers for them please. |
|
Again, I'm going to go ahead and review in the interest of saving time. This means you will need to address review comments, as well as what I discussed just above. Please stick to the process from this point. |
There was a problem hiding this comment.
First, I know you have spent some time on this and I know you are busy. Thanks for putting this effort in. The code is now in good shape.
The structure of the new section (currently called "Special note for run-time type information") is better and overall the documentation has improved a lot. However, there are still a lot of issues.
I think you need to be more thorough (and rigorous) in your self-review. For each thing that you've written - each sentence, and each paragraph - ask yourself and take the time to really think about:
- is it clear why this is discussing whatever it is discussing? (or does it need it more background/context beforehand)
- does it introduce something that hasn't been mentioned already, and if so, would it be clearer if that thing was introduced separately (perhaps in more detail) beforehand?
- is this totally accurate? (are there any exceptions to what was said)
- could it give more detail that might be useful to understanding it (that hasn't already been explained)?
I know this is time-consuming to do properly and that it is not exciting. But you need to force yourself through this process repeatedly in order for it to become natural and easy for you.
Although I was hoping to release soon, I am willing to hold off while you get this PR into shape, on the condition that you put the required time and effort into it to go through the correct process - including what I outlined above during self-review - and keep me up-to-date on progress and expected time-frame.
| You can also create and edit the "mconfig" file completely by hand (or start by copying one for a | ||
| particular OS from the "configs" directory) to choose appropriate values for the configuration | ||
| variables defined within. |
There was a problem hiding this comment.
Why are you suggesting this? What's wrong with using "make mconfig" to generate a starting point?
There was a problem hiding this comment.
I'm not suggesting that, It was there before (looks like "diff" is misleading in this case):
Lines 47 to 49 in 50cbe18
There was a problem hiding this comment.
Ok, I see what it is. It came after the small part about configure. You've removed the part about configure completely, shifting it all into a new section, then moved these lines above the section on configure, so they show up as added here and removed below.
You should keep some mention of using the configure script in this section, not completely remove it. You can make a reference to the new section.
There was a problem hiding this comment.
Changed to this:
An alternative to "make mconfig" is to use the provided "configure" script. See "configure script",
below.
You can also create and edit the "mconfig" file completely by hand (or start by copying one for a
particular OS from the "configs" directory) to choose appropriate values for the configuration
variables defined within.
EDIT: Fixes misleading diff, mentions configure outside of its own section.
There was a problem hiding this comment.
The process is to make the changes and push them to be reviewed (once questions raised in previous review are resolved), not to put the planned changes in comments. I would prefer not to clutter the review.
| An alternative to "make mconfig" is to use the provided "configure" script. It will try to | ||
| generate a suitable "mconfig" file, based on sensible defaults and options provided on the command | ||
| line when the script is run. | ||
| line when the script is run. It also has checks for the current system environment to |
There was a problem hiding this comment.
Scratch "for the current system environment", that's not adding anything useful.
based on the availability of requirements (such as headers)
Based on the available of their requirements (i.e. the requirements for those features).
Change: headers -> presence of certain header files. (Some "headers" will always be available. The features need particular headers).
One important thing that "configure" does is to check whether certain compiler options are available. That needs to be mentioned, especially as that's what you're going on to talk about next.
| The "configure" also checks for usability of the "-fno-rtti" compiler flag. This is achieved by | ||
| testing the flag itself and running a test source code to see if it works correctly. However, | ||
| because of the nature of cross-compiling, it's not possible to compile and run the test source |
There was a problem hiding this comment.
The "configure" also checks for usability of the "-fno-rtti" compiler flag
This paragraph launches in to a special case of configure behaviour without giving any background. This -fno-rtti option is a special case compared to other compilation options that are tested for, because it requires a test program to be run in order to ensure that the option functions correctly. Explain that first!! You can refer to the other section for a complete explanation, but you need to give context to understand why you are discussing something before you start discussing it.
I hope that you wrote a plan for this section, just like I suggested that you do when writing documentation.
You really need to re-write this section. Please start by developing a plan (as discussed previously) for what you need to say, review it to make sure it is cohesive (introduces topics in appropriate order), and post that (the plan) as a comment here, before you start trying to write the actual text.
There was a problem hiding this comment.
I had a plan but now it's more clear.
- An introduction to configure and how it helps user to build Dinit
- An introduction to compiler options checking
- An introduction to "-fno-rtti" compiler flag, Why it's different from other flags and how configure checks against it
- Why configure cannot check for -fno-rtti when cross-compiling
- Cross-compiling warning message
- A link to "Special note about run-time type information" for more info about "-fno-rtti"
- Pointing out configure options for build adjustments
There was a problem hiding this comment.
"how it helps user to build Dinit" - can you elaborate on what is meant by this?
"An introduction to compiler options checking" - and this?
The rest seems basically ok.
There was a problem hiding this comment.
"how it helps user to build Dinit" - can you elaborate on what is meant by this?
I mean explaining why configure is a viable option compared to make mconfig suggested earlier in the doc, because it's dynamic and this can be helpful.
"An introduction to compiler options checking" - and this?
I added this step to create contrast between -fno-rtti and other compiler options. Currently as you suggested -fno-rtti paragraph doesn't make sense because it doesn't highlight its difference from other compiler options.
There was a problem hiding this comment.
I mean explaining why configure is a viable option compared to make mconfig suggested earlier in the doc, because it's dynamic and this can be helpful.
Ok, but I don't think "dynamic" is the right word here. Also, remember that "make mconfig" actually runs configure on Linux anyway.
I added this step to create contrast between -fno-rtti and other compiler options
Ok, but what exactly will you write about for this step?
There was a problem hiding this comment.
Ok, but I don't think "dynamic" is the right word here. Also, remember that "make mconfig" actually runs configure on Linux anyway.
I mean it checks multiple things instead of just using defaults in a file (except on Linux) as explained in the first paragraph.
Ok, but what exactly will you write about for this step?
I added this step after your review and I'm working on it for the next review.
There was a problem hiding this comment.
Ok, but what exactly will you write about for this step?
I added this step after your review and I'm working on it for the next review.
That doesn't answer the question I asked. What will you write about for "An introduction to compiler options checking" - that's what's in the plan that you proposed, but it's not clear what you actually intend to write about for it. What constitutes "an introduction to compiler options checking"?
Is the previous part of the plan - "an introduction to configure and how it helps user to build Dinit" - not going to mention that configure checks if compiler options are available? If not, isn't that missing an important detail of the utility of "configure"? Or if so, what exactly is "introduction to compiler options checking" adding?
The point of the plan is to make it clear roughly what you are going to say at each step - it's not clear, though.
There was a problem hiding this comment.
That doesn't answer the question I asked. What will you write about for "An introduction to compiler options checking" - that's what's in the plan that you proposed, but it's not clear what you actually intend to write about for it. What constitutes "an introduction to compiler options checking"?
Is the previous part of the plan - "an introduction to configure and how it helps user to build Dinit" - not going to mention that configure checks if compiler options are available? If not, isn't that missing an important detail of the utility of "configure"? Or if so, what exactly is "introduction to compiler options checking" adding?
I added this step to fix weird introduction of -fno-rtti flag checking. Now I think that it should be a side note to first step instead of its own step because as you suggested without it, it's missing an important detail.
The point of the plan is to make it clear roughly what you are going to say at each step - it's not clear, though.
In addition to removing the second step, I'm going to rewrite the first step to be make it more clear.
There was a problem hiding this comment.
Ok, good, thanks.
Take a look at the example plan I gave you:
- first, introduce RTTI (what it is, what it's for, and maybe relevance to exception handling)
- 2nd, talk about "-fno-rtti", what it does exactly - including that some RTTI may still get generated for purposes of supporting exception handling, and why you might want to use "-fno-rtti"
See how it gives the detail of what will be written in each step? So it's roughly one point per paragraph.
Your original plan lacks that detail, which is why I had to ask what you intended to write (and, I think, why it doesn't really work out). You need the detail.
| =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- | ||
|
|
||
| RTTI, or run-time type information, is a feature of C++ that exposes information about an object's | ||
| data type at runtime. This can be used for "dynamic_cast<>" and to manipulate type information at |
There was a problem hiding this comment.
exposes information about an object's data type at runtime
It exposes (at runtime) information about the dynamic type of objects. ("an object" means one object).
This can be used for "dynamic_cast<>"
Just say dynamic casts. It's not valid syntax as you've written it and anyway the syntax is not important here.
You say it "can be used for" dynamic casts, but it's actually necessary for some dynamic casts. So say that. I mentioned this in the previous review.
and to manipulate type information at runtime using the "typeid" operator
How? What manipulation are you talking about? Give me an example please. (Not by pushing changes to the text).
'"typeid" operator' isn't important here. Don't discuss C++ syntax details, this is end-user documentation. Talk about the functionality (and only briefly).
GCC and Clang provide a compiler flag
"provide a compiler flag" -> "both support a flag"
which would disable RTTI generation
This isn't right.
It does (not "would") disable RTTI generation for some types.
So you can say, for example: "... which disables RTTI generation for some types".
However GCC would still generate RTTI for exception handling and therefore "-fno-rtti" is a viable option for saving output binary size
That doesn't make sense. You've said that GCC will still generate RTTI even if "-fno-rtti" is used, so it's not clear how it would reduce the size of the output binary. It needs to be made clear that less RTTI is generated (i..e RTTI for fewer types) when "-fno-rtti" is used. (That will perhaps mostly be taken care of, if you fix some of the other things I'm highlighting).
"generate RTTI for exception handling": This needs explanation/rewording. RTTI is for types, it is not "for exception handling".
You should talk about why it might be useful to avoid (or reduce) generation RTTI before you go into specifics of compiler behaviour, not afterwards.
Clang also still generates some RTTI necessary for exceptions. By only saying GCC, you seem to imply that Clang doesn't do that at all. (Do you understand the actual bug?)
Clang on some platforms (such as FreeBSD)
The platform itself is not the problem. Say what is it about those "some platforms" that makes the issue manifest.
And, if there's literally only two platforms we know of with this problem - give both. I don't understand why you would choose to highlight only one here.
There was a problem hiding this comment.
Edit:
It exposes (at runtime) information about the dynamic type of objects
It should be dynamic types of objects
There was a problem hiding this comment.
How? What manipulation are you talking about? Give me an example please. (Not by pushing changes to the text).
Hmm, after more research, I think I was misunderstood type manipulation, and I think I should remove it and only refer to some dynamic casts.
"generate RTTI for exception handling": This needs explanation/rewording. RTTI is for types, it is not "for exception handling".
I think the proper way to explain is to explain that exception types (classes) need RTTI to work.
Clang also still generates some RTTI necessary for exceptions. By only saying GCC, you seem to imply that Clang doesn't do that at all. (Do you understand the actual bug?)
I think I do. Clang does generate RTTI needed for exception types even after specifying -fno-rtti, but it's inconsistent and may break (I tested this myself before, and only the stoull() had this problem).
There was a problem hiding this comment.
Ok.
I think I do. Clang does generate RTTI needed for exception types even after specifying -fno-rtti, but it's inconsistent
(inconsistent with what)?
It sounds like you don't really understand. The issue occurs when the exception is thrown from a function compiled with -frtti (the default) and then the catch clause is in a a function compiled with -fno-rtti. The catch and throw both need RTTI but the RTTI generated will be different (a "minimal" RTTI is generated for the "no RTTI" case) and won't be deduplicated, so the catch and throw now use different RTTI and the catch doesn't recognise the thrown type and so won't catch the exception. For applications (such as Dinit) this presents a problem when they are compiled with -fno-rtti and try to catch exceptions thrown by the standard library (compiled with -frtti). This is all explained in the issue.
I tested this myself before, and only the stoull() had this problem
Likely that's the only case you tested where the exception was thrown by (non-inline code in) the standard library.
None of these details really matter, but it's not the case that Clang doesn't generate RTTI for types used as exceptions. So, again, you shouldn't imply that it is the case. I don't expect a full explanation of the bug, but whatever you do say should to be accurate.
There was a problem hiding this comment.
Hmm, after more research, I think I was misunderstood type manipulation
It worries me a lot that you wrote what you did without having some idea of what it meant. If you can't give an example of something when asked what it means you really need to do more research before you get asked about it in a review. Don't just write something if you only have a vague idea about what it means!
There was a problem hiding this comment.
(inconsistent with what)?
It sounds like you don't really understand. The issue occurs when the exception is thrown from a function compiled with -frtti (the default) and then the catch clause is in a a function compiled with -fno-rtti. The catch and throw both need RTTI but the RTTI generated will be different (a "minimal" RTTI is generated for the "no RTTI" case) and won't be deduplicated, so the catch and throw now use different RTTI and the catch doesn't recognise the thrown type and so won't catch the exception. For applications (such as Dinit) this presents a problem when they are compiled with -fno-rtti and try to catch exceptions thrown by the standard library (compiled with -frtti). This is all explained in the llvm/llvm-project#66117.
Likely that's the only case you tested where the exception was thrown by (non-inline code in) the standard library.
I mean its working is inconsistent, sometimes it does work and sometimes it doesn't. I knew that this was caused by mentioned issue but I had the idea that all exceptions to should not be caught because of that which you pointed it for me that why it didn't show in my testing.
None of these details really matter, but it's not the case that Clang doesn't generate RTTI for types used as exceptions. So, again, you shouldn't imply that it is the case. I don't expect a full explanation of the bug, but whatever you do say should to be accurate.
Agree, I will fix this.
|
|
||
| https://github.com/llvm/llvm-project/issues/66117 | ||
|
|
||
| To avoid Dinit crashes when using Clang+libcxxrt it's recommended to not use the "-fno-rtti" flag. |
There was a problem hiding this comment.
This is the very first time libcxxrt has been mentioned. This is extremely confusing. If libcxxrt is part of the issue, it should be mentioned earlier. Instead of saying something like "Clang on some platforms (such as FreeBSD)" you could be saying "Clang on platforms which also use libcxxrt as the C++ runtime library".
Finally :)
I'm going to answer your questions and fix issues raised in review tonight and tomorrow. Thanks for waiting, I think it's a good addition to next release especially enabling ioprio for more users. |
@mobin-2008 you still haven't answered this. It feels like you are ignoring it. |
I'm not ignoring that I'm just wanted to answer those after answering the current review comments. The problem is that comments outside of review comments are harder to follow for me.
I think the title is OK, because I suppose users will read Maybe for more clear title, Clang can be mentioned like this: |
| -fno-rtti : see "Special note for run-time type information", below. May cause Dinit to crash on | ||
| errors when compiled with Clang+libcxxrt on some platforms like FreeBSD. |
There was a problem hiding this comment.
I missed this in review. It's not clear. You've removed the only explanation here of why this option is recommended, and are essentially forcing the user to read the "Special note" section - even if they're not using Clang - just to find out what the option is for.
"May cause Dinit to crash" is also vague and not helpful. (To clarify: terminating because of uncaught exception is certainly a possible result, but in general, there is no guarantee that Dinit will function correctly even in the absence of "errors" if exceptions don't work properly; even if that's the case currently, it's not something I'd want to guarantee or imply. Also "cause Dinit to crash" is just the wrong way to put this, the problem is with the compiler/runtime, not with Dinit, but this makes it sound like Dinit has an issue).
"when compiled with Clang+libcxxrt on some platforms like FreeBSD" is confusing. It doesn't explain what "Clang+libcxxrt" actually means, it doesn't explain that clang with libcxxrt is the default on FreeBSD (and on MacOS), it's not clear why FreeBSD is singled out.
The text was pretty much better before this change - except that:
- we now know that the problem is caused by the combination of clang, libcxxrt, and perhaps the system dynamic linker
- it still manifests on FreeBSD (not just "historically")
- it's good that you added a reference to the new section explaining the issue in detail.
There was a problem hiding this comment.
I missed this in review. It's not clear. You've removed the only explanation here of why this option is recommended, and are essentially forcing the user to read the "Special note" section - even if they're not using Clang - just to find out what the option is for.
I got the idea from -D_GLIBCXX_USE_CXX11_ABI flag which doesn't explain the issue in itself rather explaining it in its own section but there is a problem, -D_GLIBCXX_USE_CXX11_ABI is a required flag that needs to be used on certain systems but -fno-rtti is a optional flag so it needs to explain what it does in itself. This is not good and I will fix it.
"May cause Dinit to crash" is also vague and not helpful. (To clarify: terminating because of uncaught exception is certainly a possible result, but in general, there is no guarantee that Dinit will function correctly even in the absence of "errors" if exceptions don't work properly; even if that's the case currently, it's not something I'd want to guarantee or imply. Also "cause Dinit to crash" is just the wrong way to put this, the problem is with the compiler/runtime, not with Dinit, but this makes it sound like Dinit has an issue).
It's a little tricky. I tried to minimize details as possible in user documentation to avoid confusion but this resulted in over-simplifying things. So to fix this -fno-rtti option should mention that 'it may stop Dinit from working correctly on platforms like FreeBSD and macOS. See "Special note for Clang/Libcxxrt", below for more details.' I think it's not vague anymore like it's now.
"when compiled with Clang+libcxxrt on some platforms like FreeBSD" is confusing. It doesn't explain what "Clang+libcxxrt" actually means, it doesn't explain that clang with libcxxrt is the default on FreeBSD (and on MacOS), it's not clear why FreeBSD is singled out.
This is a detail that I think should be explained in the special note. I've changed the text to only mention FreeBSD and macOS which are affected by this issue.
FreeBSD and macOS are going to mentioned with each other.
Is it necessary to mention that Clang+libcxxrt is the default compiler+runtime library on FreeBSD and macOS in this section rather in special note? I think not.
The text was pretty much better before this change - except that:
...
I think with the above fixes it will be better than previous explanation.
There was a problem hiding this comment.
Is it necessary to mention that Clang+libcxxrt is the default compiler+runtime library on FreeBSD and macOS in this section rather in special note? I think not.
The user needs to be aware whether they need to read that "special note" or note. You need to at least say that Clang together with the libcxxrt runtime is the default on "certain platforms" or something like that, but is it really so hard just to point the platforms we know about?
Do not expect that users have necessarily even heard of "libcxxrt" even if they are running MacOS or FreeBSD or another platform which uses it by default. Also, as I pointed out already, "clang+libcxxrt" is not a suitable way to refer to the combination of clang and libcxxrt in documentation.
There was a problem hiding this comment.
(Finally, just FYI, I plan on removing all mention of -D_GLIBCXX_USE_CXX11_ABI - it doesn't apply to newer versions of GCC, and I think you can no longer build with the versions where it does apply).
There was a problem hiding this comment.
Is it necessary to mention that Clang+libcxxrt is the default compiler+runtime library on FreeBSD and macOS in this section rather in special note? I think not.
The user needs to be aware whether they need to read that "special note" or note. You need to at least say that Clang together with the libcxxrt runtime is the default on "certain platforms" or something like that, but is it really so hard just to point the platforms we know about?
So it should mention 'Clang with libcxxrt runtime library which is the default compiler with runtime library on platforms like FreeBSD and macOS'.
Do not expect that users have necessarily even heard of "libcxxrt" even if they are running MacOS or FreeBSD or another platform which uses it by default. Also, as I pointed out already, "clang+libcxxrt" is not a suitable way to refer to the combination of clang and libcxxrt in documentation.
I think mentioning 'Clang with libcxxrt runtime library' is enough for this purpose. Also I think 'Clang with libcxxrt runtime library' is better than "clang+libcxxrt".
This is not necessarily a valid assumption. They might just skip over sections that don't sound important/relevant to them.
I actually missed that 2nd reference in the review; I added some new comments just now.
The title is: "Special note for run-time type information". It only is important to read if you are using Clang together with libcxxrt. Changing the title to refer to Clang (with libcxxrt) rather than "run-time type information" makes that clear.
You should assume that people building Dinit do not necessarily know C++. They may not know what "run-time type information" is, so those words have no meaning for them until it is explained. The title should focus on the important detail as far as the user is concerned. |
|
Also:
That question first appeared in a review comment, though, but you hadn't responded to it, so I don't really accept this. If it's hard for you to follow comments outside review then (1) respond to them as part of the proper, documented process before pushing new commits so this doesn't happen and (2) sorry, but you still need to respond to comments outside review anyway. I don't want this to drag on forever, so you need to respond as promptly as possible to all the feedback. I would appreciate if you do this top-to-bottom rather than addressing the most recent review first. If you're only responding to some things but know there are other things to get back, then make it clear that you know you still need to get back to those things. Otherwise, it certainly looks like you are ignoring them or have forgotten about them. With all that said: I think the only oustanding question before you can start to work on changes is this one - #489 (comment) - unless you have questions or any disagreements. |
What I meant was that it wasn't in a GitHub-style review comment which points to a specific line in the changes and after a quick review of previous comments, I think that's my fault because I posted the actual change as a comment and shifted the conversation to regular comments which are harder to me to follow.
I agree. What I meant was that having multiple threads at the same time would be harder for me to follow when it's hard to find them in the conversation (GitHub-style review comments don't have this problem) and as you mentioned in the beginning:
Now I think this should have been 3 PRs to avoid multiple threads from rising up in a single PR.
I really appreciate your effort on review comments and in addition to mentioned things I will make clear that I'm going to get back for regular comments.
I'm going to answer that in a short time. |
Commits are self-explanatory.
The first commit is the base for all new checks.
The second commit is a nice touch I think.
The third commit Fixes #478
The last commit Fixes #488