Skip to content

Conversation

@firewave
Copy link
Collaborator

No description provided.

@firewave firewave changed the title Iwyu.yml: made include-what-you-use output more verbose iwyu.yml: made include-what-you-use output more verbose Oct 10, 2023
@firewave firewave changed the title iwyu.yml: made include-what-you-use output more verbose iwyu.yml: made include-what-you-use output more verbose / cleaned up includes Oct 10, 2023
@firewave firewave force-pushed the iwyu branch 3 times, most recently from 704c4fc to 225d24a Compare October 12, 2023 11:47
@firewave firewave marked this pull request as ready for review October 12, 2023 11:48
@firewave firewave marked this pull request as draft October 12, 2023 12:23
#endif


// TODO: __USE_DYNAMIC_STACK_SIZE is depedent on the features.h include and not a built-in compiler define, so it might be problematic to depedent on it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like features.h should be included then? Also, 2x typo depedent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to figure out what is actually including this as it seems to be available in all context - I assume that might be caused by the precompiled headers. It is also not available on all platforms so there's a chance it will be breaking some builds (e.g. MacOS does not have it) so it needs some preprocessor checks. I can't be bothered to do any of that right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, 2x typo depedent.

I will fix those in another PR with several other typos.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears features.h is included if you include any system header. __USE_DYNAMIC_STACK_SIZE is defined and set to 1 if _DYNAMIC_STACK_SIZE_SOURCE is defined. That in turn is defined if the user specified _GNU_SOURCE.

features.h documents the following:

__USE_DYNAMIC_STACK_SIZE Define correct (but non compile-time constant)
                         MINSIGSTKSZ, SIGSTKSZ and PTHREAD_STACK_MIN.

But if you look at our code it seems like the check is actually backwards:

#ifdef __USE_DYNAMIC_STACK_SIZE
static const size_t MYSTACKSIZE = 16*1024+32768; // wild guess about a reasonable buffer
#else
static const size_t MYSTACKSIZE = 16*1024+SIGSTKSZ; // wild guess about a reasonable buffer
#endif

And if you change that code it does not even compile:

cli/cppcheckexecutorsig.cpp:58:13: error: variable length array declaration not allowed at file scope
static char mytstack[MYSTACKSIZE]= {0}; // alternative stack for signal handler
            ^        ~~~~~~~~~~~

Checking the documentation of stack_t where this is being used:
https://www.gnu.org/software/libc/manual/html_node/Signal-Stack.html

size_t ss_size
This is the size (in bytes) of the signal stack which ‘ss_sp’ points to. You should set this to however much space you allocated for the stack.

There are two macros defined in signal.h that you should use in calculating this size:

SIGSTKSZ
This is the canonical size for a signal stack. It is judged to be sufficient for normal uses.

MINSIGSTKSZ
This is the amount of signal stack space the operating system needs just to implement signal delivery. The size of a signal stack must be greater than this.

For most cases, just using SIGSTKSZ for ss_size is sufficient. But if you know how much stack space your program’s signal handlers will need, you may want to use a different size. In this case, you should allocate MINSIGSTKSZ additional bytes for the signal stack and increase ss_size accordingly.

So we are allocating way too much memory for the stack.

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/sigaltstack.2.html shows an example on how to allocate it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also https://reviews.llvm.org/D28265 indicates that using sigaltstack() on MacOS breaks backtrace().

@firewave firewave mentioned this pull request Oct 16, 2023
@chrchr-github chrchr-github merged commit 5e89eb0 into danmar:main Oct 16, 2023
@firewave firewave deleted the iwyu branch October 16, 2023 11:07
firewave added a commit that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants