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

Possible overflow when using BLOCK_BUFFER_SIZE >= 128 #20130

Merged
merged 7 commits into from
Nov 16, 2020
Merged

Possible overflow when using BLOCK_BUFFER_SIZE >= 128 #20130

merged 7 commits into from
Nov 16, 2020

Conversation

FanDjango
Copy link
Contributor

Possible overflow when using BLOCK_BUFFER_SIZE >= 128

When defining a BLOCK_BUFFER_SIZE >= 128 in configuration_adv.h in combination with using #define DISABLE_INACTIVE_EXTRUDER in configuration.h, there will be a compiler warning (with possible unwanted consequences) as follows:

  438 |       static uint8_t g_uc_extruder_last_move[EXTRUDERS];
      |                      ^~~~~~~~~~~~~~~~~~~~~~~
In file included from Marlin\src\module\../inc/../core/boards.h:24,
                 from Marlin\src\module\../inc/MarlinConfigPre.h:37,
                 from Marlin\src\module\../inc/MarlinConfig.h:28,
                 from Marlin\src\module\../MarlinCore.h:24,
                 from Marlin\src\module\planner.h:33,
                 from Marlin\src\module\planner.cpp:65:
Marlin\src\module\planner.cpp: In static member function 'static bool Planner::_populate_block(block_t*, bool, const abce_long_t&, const xyze_pos_t&, feedRate_t, uint8_t, const float&)':
Marlin\src\module\planner.cpp:2052:62: warning: unsigned conversion from 'int' to 'uint8_t' {aka 'unsigned char'} 
changes value from '256' to '0' [-Woverflow]
 2052 |             g_uc_extruder_last_move[N] = (BLOCK_BUFFER_SIZE) * 2; \
      |                                          ~~~~~~~~~~~~~~~~~~~~^~~

Changing the definition of g_uc_extruder_last_move from uint8_t to uint16_t in planner.cpp/planner.h would solve this and this won't cost very much memory (i.e. plus one byte per extruder).

Possible overflow then using BLOCK_BUFFER_SIZE >= 128
@thinkyhead
Copy link
Member

I'm not sure we want to advocate such a large buffer size. There's no special benefit to a very large buffer. I may just add a sanity check for buffer size larger than 64.

thinkyhead and others added 2 commits November 13, 2020 20:14
@FanDjango
Copy link
Contributor Author

FanDjango commented Nov 14, 2020

Sanity check would be sufficient, then the conditional type would not even be needed.

Need to modify the comment in configuration_adv imho.

@thinkyhead
Copy link
Member

I don't want to make it a hard policy, per se. I do, however, want to suggest that for most users there is not a need to use such a large buffer.

@thinkyhead thinkyhead merged commit 110e0d7 into MarlinFirmware:bugfix-2.0.x Nov 16, 2020
@FanDjango FanDjango deleted the planner_overflow branch November 17, 2020 09:39
FhlostonParadise pushed a commit to FhlostonParadise/Marlin that referenced this pull request Nov 21, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Kannix2005 pushed a commit to Kannix2005/Marlin-1 that referenced this pull request Dec 7, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
tharts pushed a commit to tharts/Marlin that referenced this pull request Jan 6, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
dpreed pushed a commit to dpreed/Marlin_2.0.x that referenced this pull request Feb 5, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 28, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
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.

2 participants